Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify that repr should make julia code #30757

Closed
wants to merge 2 commits into from

Conversation

oxinabox
Copy link
Contributor

The docs currently are descriptive, they say "this usually make",
this changers them to be perscriptive, saying "this should make".

A problem with this is that the docstring under this PR is now written to be read by an implementer,
not by a user.
An alternative would be to put this presriptive statment in a Note to Implementors
bit at the bottom.

Here is the equivalent python docs

object.repr(self)

Called by the repr() built-in function to compute the “official” string representation of an object. If at all possible, this should look like a valid Python expression that could be used to recreate an object with the same value (given an appropriate environment). If this is not possible, a string of the form <...some useful description...> should be returned. The return value must be a string object. If a class defines repr() but not str(), then repr() is also used when an “informal” string representation of instances of that class is required.

This is typically used for debugging, so it is important that the representation is information-rich and unambiguous.

@omus this is re: our discussion yesterday.

base/strings/io.jl Outdated Show resolved Hide resolved
@tkf
Copy link
Member

tkf commented Jan 19, 2019

Is repr supposed be a call-only API? Or is it overloadable?

  • If repr is a call-only API:

    • Then, since its definition is repr(x; context=nothing) = sprint(show, x; context=context), I think docstring of show has to mention that "show(::IO, ::YourType) MUST print valid Julia code."
    • But show(io, x) is often used as "short" pretty-printing method (while show(io, "text/plain", x) is a "long" pretty-printing method). For example, display([T()]) calls show(::IO, ::T), not show(::IO, ::MIME"text/plain", ::T) (see previous discussion in discourse). If this is intended, I suggest to use show(::IO, ::MIME"application/julia", ::T) in repr with the fallback to show(::IO, ::T) (which can be done with showable) and mention this behavior in repr's # Implementation section. One of the benefit of introducing a new mime type application/julia in the show convention is that you get a "strict" repr by directly calling sprint(show, "application/julia", x) which fails if application/julia is not defined for object x and its sub-objects if x is a container.
  • If repr is a overloadable API:

    • I suggest to add that "You MUST implement repr if show(::IO, ::YourType) prints invalid Julia code" in # Implementation section of show, in addition to repr.

@oxinabox
Copy link
Contributor Author

Everything is overloadable in Julia.

I think yes, the guidance of having something like
You MUST implement repr if show(::IO, ::YourType) prints invalid Julia code"
is along the right lines.
idk about MUST, vs should if at all possible

@JeffBezanson
Copy link
Member

repr should never be overloaded. It's defined to give a string of whatever 2-argument show prints. We have

  • minimal/canonical representation: print. string gives a string of its output.
  • parseable (if possible) representation of any value: show(io, x). repr(x) gives a string of its output.
  • pretty display format for any value: show(io, mime, x). repr(mime, x) gives a string of its output.

I believe that's enough different output formats, and there is no real use for repr and 2-argument show giving different results.

@JeffBezanson
Copy link
Member

The one main exception to that is it's sometimes ok to directly define string, if converting a certain type to strings is important and can be done much faster than going via the print-to-string fallback.

@oxinabox
Copy link
Contributor Author

oxinabox commented Jan 23, 2019

I like those pair definitions.
Though I wonder if print(String, x) would be nicer than string and such.
(Not a discussion for this PR though)

Anyway, outcome of this discussion is that

  • repr's docstring needs updating to say: Never loverload this, instead overload show.
  • shows docstring needs more extensive rewriting to talk about parsable if possible, since it doesn't mention parsable at all ruight now . (It says julia specific formatting, which is probably a reference to that, but it is unclear terminology (to me at least))

@JeffBezanson
Copy link
Member

JeffBezanson commented Jan 23, 2019

I agree with those proposed edits.

Though I wonder if print(String, x) would be nicer than string and such.

We have that, as sprint(show, x), sprint(print, x) etc. but they are way too long and annoying to write, so some standard shortcuts are nice to have.

@oxinabox
Copy link
Contributor Author

my wonders are about clarity.

Until you mentioned it it was not obvious to me that it was a core part of the definition of
string <=> print
repr <=> show

I mean I knew they did that. But I thought of it as a fallback behavour not as core to what they were.

@tkf
Copy link
Member

tkf commented Jan 23, 2019

@JeffBezanson How about adding show(::IO, ::MIME"application/julia", ::T) convention for output that must be parsable?

@JeffBezanson
Copy link
Member

That might be ok, but that would mean you have a type where it's possible to show it in a parseable way, but show does something else --- why? For non-parseable representations there's text/plain.

@JeffBezanson
Copy link
Member

I should add that we are not really strict about show output needing to be parseable. I think the reason for that is that I don't really see many use cases for parseable output, but I could be wrong. If there are contexts where parseable output is really important, we indeed might need to add a new thing for guaranteed parseable output.

@tkf
Copy link
Member

tkf commented Jan 24, 2019

but that would mean you have a type where it's possible to show it in a parseable way, but show does something else --- why? For non-parseable representations there's text/plain.

One of the reason is that text/plain with container like display([x]) and display(Dict(:a=>x)) calls show without mime of element x. So, you'd still want to pretty-print even with 2-arg show. But this probably has to be fixed at the level of container's show implementations (e.g., maybe they should use summary or show(io, "text/plain", x)).

It's also discussed in https://discourse.julialang.org/t/how-to-detect-in-show-that-you-are-computing-display-for-the-repl/18951

use cases for parseable output

One thing I have in mind is example-based testing like doctest. It'd be useful if I can dump the repr of one version as a (semi-)readable text file so that it's git diff'able when some change occurs. It then can be includeed during tests to be compared with the current implementation. Of course, it can be done with more standard de/serialization like JSON so I'm not sure how useful it would be. But I can imagine that it can be robust against change in implementation if you use "show it like you build it" approach and print "factory function" like view instead of SubArray.

@nickrobinson251
Copy link
Contributor

Bump? Seems like a helpful docstring improvement as is :)

@oxinabox
Copy link
Contributor Author

It needs to be moved to be on the show, not repr.
and repr needs the docstring saying "don't overload this, just overload show.

@User-764Q
Copy link

This issue has been open for a while,

Its it worth keeping it open or can it be closed?

Matt.

@oxinabox
Copy link
Contributor Author

It is still valid last I checked.
Someone need to go and make the edits Jeff suggestef to the docstring for show (and prob also summarise them in the repr docstring)

@ViralBShah ViralBShah added the docs This change adds or pertains to documentation label Feb 25, 2022
@vtjnash vtjnash closed this Feb 10, 2023
@vtjnash
Copy link
Member

vtjnash commented Feb 10, 2023

Looks like there are some proposed other updates (#30757 (comment)), but the specific changes proposed here were not in line with the behavior expected of this function

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs This change adds or pertains to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants