-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Added additional formatting options for slices #44751
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 @alexcrichton (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
Rustforge has some documentation about creating a new feature: https://forge.rust-lang.org/feature-guide.html |
Is this PR big enough to warrent a feature-flag? What is the usual procedure for small library changes? |
Thanks for the PR @Gilnaa! Unfortunately stability attributes right now don't cover trait implementations, so this would be "insta stable" in the sense of we can't mark this unstable to start out. In any case though this looks great! I'm personally a fan of these implementations. Could you update the The next step here would then be to get a sign-off from the @rust-lang/libs team for approval, and to do that I'll... @rfcbot fcp merge |
Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged teams: No concerns currently listed. Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
You have a typo in your PR title (and maybe elsewhere?): |
@alexcrichton Fixed |
It's understood that this will use the formatting options once per element? Just checking. For example padding, field width etc is per element, not for the whole slice. For the record I think that's ok and useful and it's the one issue that has been "unresolved" before adding this kind of adaptor for an iterator (basically itertools's format into libstd). |
src/libcore/fmt/mod.rs
Outdated
|
||
for o in self.iter() { | ||
if has_items { | ||
write!(f, ", ")?; |
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 can be f.write_str to avoid some dynamic dispatch.
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.
(Same with the other single strings)
@bluss Yes, the padding information is carried to each element using the Formater instance. Edit: You can actually see an example in the test function |
I think that's good. That kind of padding/width behaviour is indispensable in for example ndarray, to format a matrix of numbers in a readable way. The reminder was intended for everyone deciding on the issue, since that aspect hasn't been discussed yet (that I can see). |
What about This is committing to a completely arbitrary choice of surrounding with "[]" and separating with ", " which isn't what #33127 or #41674 wanted. What I'd personally like to see is a nice way to format items in iterators in general like |
I kind of agree with @ollie27 - I would expect |
@ollie27 I don't know about display. I have no idea what was the original reasoning not to impl it for slices, but I assume there was one. @sfackler That would either require all [T] to be renderd like this, which is not ideal, or have a specialization for [u8], but that would cause inconsistency and violate the principle of least surprise. Of course, I completely agree that there ought to do this hexdump-esque operation, but the domain of format specifiers is not that large. |
I agree with @ollie27 that this is a confusingly arbitrary choice of behavior to assign to format!("{:#x}", CommaSeparated(slice)) == "0x0, 0x1, 0x2"
format!("[{:#x}]", CommaSeparated(slice)) == "[0x0, 0x1, 0x2]"
format!("{:02x}", Concatenated(slice)) == "000102" |
I understand the motivation, but this just leave slice (directly) un-formattable using these traits? |
Ok we got a chance to discuss this during the libs triage meeting today, and our conclusion for now was that we probably don't actually want to merge it. As evidenced from the discussion it's not clear what the semantics should be here of these traits implemented directly on slices. @dtolnay's suggestion as well could also possibly be turned into an entire library dedicated to providing high-quality formatting-related utilities for slices and other datatypes, handling information like formatting flags, customization, etc. Our feeling was that we probably in the long run want a sort of "adapter" to implement this functionality in the standard library, one that you have to reach for and explicitly ask for so you know what it's doing. This we felt was best prototyped outside the standard library before moving in. In light of all that I'm going to close this for now, but thanks regardless for the PR @Gilnaa! |
First PR, sorry if anything is out of place.
Specifically I didn't have any idea about what to write in the "unstable" attribute, but I improvised and it seems to compile.
This PR tackles 1916.
I thought about writing something similar to what builders::DebugInner does, but in the implementation for the rest of the traits, the Alternate flag is already taken (e.g. # in Debug is pretty-print, # in Octal is
0o
).Cheers