-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Edit of "Output-function summary" docs from a new learner #56767
base: master
Are you sure you want to change the base?
Conversation
Minor changes first
Larger reorganization of Output-function summary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for offering this edit, @nathanrboyer!
Personally I have no preference for the proposed version over the previous version or vice versa. I'd be happy to help do a final review and merge if other folks think this is an improvement, though.
doc/src/manual/types.md
Outdated
Note that [`convert(String, x)`](@ref) is purposely not defined | ||
so the user can be explicit about the string representation they desire. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that the real reason is that convert
in Julia is used for implicit conversions, e.g. when assigning an expression to an container of a different type. So it is never defined for converting between conceptually different types of objects, e.g. between numbers and strings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have now rewritten this sentence according to your explanation.
doc/src/manual/types.md
Outdated
* [`show(io, mime, x)`](@ref), with three arguments, | ||
performs verbose pretty-printing of `x`. | ||
Multiple 3-argument `show` methods can be defined for various MIME types to enable | ||
richer display of `x` in some interactive environments as discussed above. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
richer display of `x` in some interactive environments as discussed above. | |
richer display of `x` in some interactive environments as discussed below. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, when I read this passage I thought "above" referred to the Polar
example in the previous section. Now I think you may have been referring to the small explanation in the display
bullet. I will rethink this.
doc/src/manual/types.md
Outdated
Multiple 3-argument `show` methods can be defined for various MIME types to enable | ||
richer display of `x` in some interactive environments as discussed above. | ||
By default (if no 3-argument method is defined for `typeof(x)`), | ||
it calls the 2-argument `show(io, x)`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it calls the 2-argument `show(io, x)`. | |
`show(io, "text/plain", x)` calls the 2-argument `show(io, x)`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sentence is copied from the original text, but I am thinking now it can just be deleted?
If not deleted, then maybe moved into the display
bullet like this?
In the REPL,
display
callsshow(io, MIME"text/plain", x)
(which defaults toshow(io, x)
if not separately defined).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s copied from the previous text, but you edited the preceding sentence so that the new qualification is needed.
I don’t think it can be deleted. It’s important to explain how 3-arg show defaults to 2-arg show (but only for text/plain)
is the default simple text representation of `x`. | ||
It is typically the format you would employ to input `x` into Julia. | ||
|
||
* [`show(io, mime, x)`](@ref), with three arguments, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* [`show(io, mime, x)`](@ref), with three arguments, | |
* [`show(io, ::MIME"text/plain", x)`](@ref), with three arguments, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not understand why you want this bullet to be specific to a text representation rather than explaining the 3-argument show
generally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because text/plain is the common case, and most of this section is about text output . Most types don’t offer output for any other mime types, which is why they are only mentioned in passing here.
Also, if you want to overload it you need to know the specific signature.
doc/src/manual/types.md
Outdated
|
||
* [`show(io, mime, x)`](@ref), with three arguments, | ||
performs verbose pretty-printing of `x`. | ||
Multiple 3-argument `show` methods can be defined for various MIME types to enable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Multiple 3-argument `show` methods can be defined for various MIME types to enable | |
Additional 3-argument `show` methods can be defined for other MIME types to enable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is contingent on the decision above: mime
vs ::MIME"text/plain"
.
doc/src/manual/types.md
Outdated
performs verbose pretty-printing of `x`. | ||
Multiple 3-argument `show` methods can be defined for various MIME types to enable | ||
richer display of `x` in some interactive environments as discussed above. | ||
By default (if no 3-argument method is defined for `typeof(x)`), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By default (if no 3-argument method is defined for `typeof(x)`), | |
Although the underlying method is defined for a [`MIME`](@ref) argument, the caller can | |
simply pass a string such as `"text/plain"`, at the expense of a dynamic dispatch. | |
By default (if no 3-argument method is defined for `typeof(x)`), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is good to explain, but it leaves me with more questions than answers. Why is the string version used most places if it is slower?
See also this related issue I created: #56768
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Internally, we should call the MIME version. For users, the string version is easier, and is rarely performance-critical.
See also #55860 for another tweak. |
Co-authored-by: Steven G. Johnson <[email protected]>
Co-authored-by: Steven G. Johnson <[email protected]>
Co-authored-by: Steven G. Johnson <[email protected]>
I added a form of that PR as below.
|
@stevengj I took another pass at the summary. The bullets are longer now but hopefully more informative. Please review at your convenience. My intent was to start by explaining each function/method generally and then suggest to the reader how/why they might wish to create new method definitions. |
New types will automatically define `show(io, x)` | ||
as the format employed to input `x` into Julia. | ||
However, it may be desirable to overwrite this default | ||
to define nicer printing for collections of the new type. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not totally confident on why when/why to overwrite 2-arg show. Printing inside a vector is the example used above for Polar
, but the show
docstring maintains that the definition "should be parseable Julia code when possible" which seems to already be the default definition.
This is a refactor of the helpful output summary written recently by @stevengj in #54547. I think it makes the section more digestible to move some of the information that was contained within the bullets into separate parts, so the bullet points themselves are smaller. It may need to be edited for accuracy and formality.