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

Make fmt::DebugList and friends forward formatting parameters #46233

Merged
merged 2 commits into from Dec 20, 2017

Conversation

Projects
None yet
8 participants
@SimonSapin
Contributor

SimonSapin commented Nov 24, 2017

For example, formatting slice of integers with {:04?} should zero-pad each integer.

This also affects every use of #[derive(Debug)].

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Nov 24, 2017

Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @sfackler (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.

Collaborator

rust-highfive commented Nov 24, 2017

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @sfackler (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.

Make fmt::DebugList and friends forward formatting parameters
For example, formatting slice of integers with `{:04?}`
should zero-pad each integer.
@sfackler

This comment has been minimized.

Show comment
Hide comment
@sfackler

sfackler Nov 28, 2017

Member

Why would padding be applied to elements and not the list as a whole?

Member

sfackler commented Nov 28, 2017

Why would padding be applied to elements and not the list as a whole?

@SimonSapin

This comment has been minimized.

Show comment
Hide comment
@SimonSapin

SimonSapin Nov 28, 2017

Contributor

It’s… better than nothing?

I assume you mean space-padding. For what it’s worth, I mostly wanted this for zero-padding for example u32’s to 8 hexadecimal digits with rust-lang/rfcs#2226

And padding is just an example, this forwards other formatting parameters like plus sign and precision too.

Contributor

SimonSapin commented Nov 28, 2017

It’s… better than nothing?

I assume you mean space-padding. For what it’s worth, I mostly wanted this for zero-padding for example u32’s to 8 hexadecimal digits with rust-lang/rfcs#2226

And padding is just an example, this forwards other formatting parameters like plus sign and precision too.

@sfackler

This comment has been minimized.

Show comment
Hide comment
@sfackler

sfackler Nov 28, 2017

Member

Hmm, not sure how I feel about pushing this kind of thing down, but I don't think we have any precedent either way.

Thoughts @rust-lang/libs?

@rfcbot fcp merge

Member

sfackler commented Nov 28, 2017

Hmm, not sure how I feel about pushing this kind of thing down, but I don't think we have any precedent either way.

Thoughts @rust-lang/libs?

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Nov 28, 2017

Team member @sfackler 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 commented Nov 28, 2017

Team member @sfackler 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.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Nov 29, 2017

Member

I share @sfackler's wariness here in that this implementation seems almost guaranteed for someone 6 months from now to open a bug saying "this is implementation is confusing and not obvious". It definitely isn't really respecting the various formatting flags in the same way types like str do (in that it's for the whole output not for each element).

That being said these are Debug (emphasis on debug) impls and are in general a little looser around requirements than other implementations. I could also imagine how these are certainly convenient when you want this behavior (which seems like you'd want this behavior more often than "do the flags for the whole output" behavior).

I do think we should clarify in documentation what's happening here. Either on std::fmt documentation or on the debug builder methods themselves.

Member

alexcrichton commented Nov 29, 2017

I share @sfackler's wariness here in that this implementation seems almost guaranteed for someone 6 months from now to open a bug saying "this is implementation is confusing and not obvious". It definitely isn't really respecting the various formatting flags in the same way types like str do (in that it's for the whole output not for each element).

That being said these are Debug (emphasis on debug) impls and are in general a little looser around requirements than other implementations. I could also imagine how these are certainly convenient when you want this behavior (which seems like you'd want this behavior more often than "do the flags for the whole output" behavior).

I do think we should clarify in documentation what's happening here. Either on std::fmt documentation or on the debug builder methods themselves.

@kennytm

This comment has been minimized.

Show comment
Hide comment
@kennytm
Member

kennytm commented Dec 6, 2017

@kennytm

This comment has been minimized.

Show comment
Hide comment
@kennytm
Member

kennytm commented Dec 13, 2017

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Dec 19, 2017

🔔 This is now entering its final comment period, as per the review above. 🔔

rfcbot commented Dec 19, 2017

🔔 This is now entering its final comment period, as per the review above. 🔔

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Dec 19, 2017

Member

@bors: r=sfackler

Member

alexcrichton commented Dec 19, 2017

@bors: r=sfackler

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 19, 2017

Contributor

📌 Commit bf08789 has been approved by sfackler

Contributor

bors commented Dec 19, 2017

📌 Commit bf08789 has been approved by sfackler

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 20, 2017

Contributor

⌛️ Testing commit bf08789 with merge a5f1195...

Contributor

bors commented Dec 20, 2017

⌛️ Testing commit bf08789 with merge a5f1195...

bors added a commit that referenced this pull request Dec 20, 2017

Auto merge of #46233 - SimonSapin:fmt-debuglist-flags, r=sfackler
Make fmt::DebugList and friends forward formatting parameters

For example, formatting slice of integers with `{:04?}` should zero-pad each integer.

This also affects every use of `#[derive(Debug)]`.
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 20, 2017

Contributor

💔 Test failed - status-travis

Contributor

bors commented Dec 20, 2017

💔 Test failed - status-travis

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Dec 20, 2017

Member

@bors: retry

  • wedged download?
Member

alexcrichton commented Dec 20, 2017

@bors: retry

  • wedged download?
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 20, 2017

Contributor

⌛️ Testing commit bf08789 with merge 6dbf0ba...

Contributor

bors commented Dec 20, 2017

⌛️ Testing commit bf08789 with merge 6dbf0ba...

bors added a commit that referenced this pull request Dec 20, 2017

Auto merge of #46233 - SimonSapin:fmt-debuglist-flags, r=sfackler
Make fmt::DebugList and friends forward formatting parameters

For example, formatting slice of integers with `{:04?}` should zero-pad each integer.

This also affects every use of `#[derive(Debug)]`.
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 20, 2017

Contributor

☀️ Test successful - status-appveyor, status-travis
Approved by: sfackler
Pushing 6dbf0ba to master...

Contributor

bors commented Dec 20, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: sfackler
Pushing 6dbf0ba to master...

@bors bors merged commit bf08789 into rust-lang:master Dec 20, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@SimonSapin SimonSapin deleted the SimonSapin:fmt-debuglist-flags branch Feb 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment