Skip to content

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

Closed
wants to merge 3 commits into from
Closed

Added additional formatting options for slices #44751

wants to merge 3 commits into from

Conversation

Gilnaa
Copy link

@Gilnaa Gilnaa commented Sep 21, 2017

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

@rust-highfive
Copy link
Contributor

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.

@mattico
Copy link
Contributor

mattico commented Sep 22, 2017

Rustforge has some documentation about creating a new feature: https://forge.rust-lang.org/feature-guide.html

@Gilnaa
Copy link
Author

Gilnaa commented Sep 22, 2017

Is this PR big enough to warrent a feature-flag? What is the usual procedure for small library changes?

@alexcrichton alexcrichton added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Sep 22, 2017
@alexcrichton
Copy link
Member

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 #[unstable] tag to #[stable] to reflect how they'll be stable on landing?

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

@rfcbot
Copy link
Collaborator

rfcbot commented Sep 22, 2017

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.

@rfcbot rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Sep 22, 2017
@shepmaster
Copy link
Member

You have a typo in your PR title (and maybe elsewhere?): addtional. I'll fix the PR title.

@shepmaster shepmaster changed the title Added addtional formatting options for slices Added additional formatting options for slices Sep 22, 2017
@Gilnaa
Copy link
Author

Gilnaa commented Sep 22, 2017

@alexcrichton Fixed
@shepmaster Right, thanks :) . I couldn't find any more typos.

@bluss
Copy link
Member

bluss commented Sep 23, 2017

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).


for o in self.iter() {
if has_items {
write!(f, ", ")?;
Copy link
Member

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.

Copy link
Member

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)

@Gilnaa
Copy link
Author

Gilnaa commented Sep 23, 2017

@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

@bluss
Copy link
Member

bluss commented Sep 23, 2017

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).

@ollie27
Copy link
Member

ollie27 commented Sep 23, 2017

What about Display? What about other collections?

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 itertools::Itertools::format does.

@sfackler
Copy link
Member

I kind of agree with @ollie27 - I would expect {:x} to just make a hex string for a byte slice if anything.

@Gilnaa
Copy link
Author

Gilnaa commented Sep 25, 2017

@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.
Maybe create somekind of wrapper newtype the implements it differently?

@dtolnay
Copy link
Member

dtolnay commented Sep 25, 2017

I agree with @ollie27 that this is a confusingly arbitrary choice of behavior to assign to {:x} and friends. I would prefer not to accept this, and solve it with adapter types in an external library instead.

format!("{:#x}", CommaSeparated(slice)) == "0x0, 0x1, 0x2"

format!("[{:#x}]", CommaSeparated(slice)) == "[0x0, 0x1, 0x2]"

format!("{:02x}", Concatenated(slice)) == "000102"

@Gilnaa
Copy link
Author

Gilnaa commented Sep 25, 2017

I understand the motivation, but this just leave slice (directly) un-formattable using these traits?

@alexcrichton
Copy link
Member

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants