-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Document that Display entails ToString and should be lossless and infallible #94488
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
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @m-ou-se (or someone else) soon. Please see the contribution instructions for more information. |
I don't think this is universally true. In the But for complex aggregate types which don't implement |
From the thread:
This implies that the String returned by |
That kind of seems backwards to me. If it is user-facing (as the documentation says) then there should be no assumption about Again, for simple types representing just one thing I kind of can see it. But for complex types I would apply different heuristics. I guess we can discuss this somewhere else, I just want to register my general disagreement. |
I agree that it's a bit confusing. For me, it clears things up to think of this as applying only with the context of the standard library's formatter. The |
/// `Display` is similar to [`Debug`], but `Display` is for user-facing output, | ||
/// and so cannot be derived. When `Display` is implemented, the [`ToString`] |
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.
/// `Display` is similar to [`Debug`], but `Display` is for user-facing output, | |
/// and so cannot be derived. When `Display` is implemented, the [`ToString`] | |
/// `Display` is similar to [`Debug`], but `Display` is for user-facing output. | |
/// When `Display` is implemented, the [`ToString`] |
As long as we're fixing this documentation: this conclusion doesn't actually automatically follow, and might not always be true.
Agreed; in particular, the "lossless" phrasing might encourage people to produce Display impls that don't do as good a job at being human-readable, because they over-focus on round-tripping. |
Ping from triage: FYI: when a PR is ready for review, send a message containing |
@rustbot ready Sorry I've been out for a long time but I'm back in the office today. FYI, the Book gives the impression that
|
/// trait is implemented automatically, adding the `to_string` method to all | ||
/// `Display` types. | ||
/// | ||
/// A `Display` implementation should be lossless and infallible, otherwise it |
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.
How do you define lossless here? Any definition I can think of feels way too strict..
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.
"doesn't lose information from its internal data" and then we can use Path
as an example?
/// A `Display` implementation should be lossless and infallible, otherwise it | ||
/// does not fit the API. For example, converting a path to a [`String`] is | ||
/// potentially lossy or fallible, so [`Path`] doesn’t implement `Display` | ||
/// directly (but offers a .display() method). |
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.
/// directly (but offers a .display() method). | |
/// directly (but offers a `.display()` method). |
☔ The latest upstream changes (presumably #102331) made this pull request unmergeable. Please resolve the merge conflicts. |
@rustbot author Once you're done doing everything, type in |
Closing this pr due to inactivity. If you wish you can reöpen this PR or create a new one when you have the time for it and we can take it forward from there |
#92941
Taking what was said here and in the issue itself, I tried to be concise and explicit. I kept my lines to 80 characters according to the guide.
I want to be able to write docs like the ones in
std
so any feedback is appreciated. This is also my first contribution so I apologize if I made mistakes in the submission process.