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

SimonSapin · 2017-11-24T08:59:41Z

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

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

rust-highfive · 2017-11-24T08:59:53Z

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.

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

bluss · 2017-11-25T00:17:59Z

src/libcore/fmt/builders.rs

+                writer.write_str("\n")?;
+                writer.write_str(name)?;
+                writer.write_str(": ")?;
+                value.fmt(&mut writer.as_formatter())


What's the difference between using this and value.fmt(writer.fmt) here?

That would break the indentation and newline insertion that PadAdapter is there to do, which is particularly useful for deeply nested structures.

I see! I thinking we could have written this without privileged access to formatter internals, but this way has less indirections and in debug struct it really benefits everyone, so I think it serves its purpose.

I tried making this a method of Formatter instead, but couldn’t borrow &mut writer and &mut writer.fmt at the same time.

Alright, I found a way to make it work (in a new commit) but it’s a bit messy with up to three named lifetimes in the same item. I’m not sure that it’s better than before. What do you think?

I think it looks quite neat. I'd pinpoint two places to place comments and it's good for me: 1. What's the slot argument?, 2. One sentence explaining the wrap_buf method. (The comment "We want to change this" is a sign that a prior brief comment is missing IMO)

sfackler · 2017-11-28T04:26:50Z

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

SimonSapin · 2017-11-28T07:48:24Z

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 · 2017-11-28T18:01:47Z

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 · 2017-11-28T18:01:47Z

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 · 2017-11-29T00:19:47Z

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 · 2017-12-06T12:54:05Z

Ping @BurntSushi, @aturon, ticky boxes for you!

kennytm · 2017-12-13T09:47:38Z

Ping² @BurntSushi, @aturon, ticky boxes for you!

rfcbot · 2017-12-19T21:23:46Z

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

alexcrichton · 2017-12-19T22:12:09Z

@bors: r=sfackler

bors · 2017-12-19T22:12:09Z

📌 Commit bf08789 has been approved by sfackler

bors · 2017-12-20T00:41:59Z

⌛ Testing commit bf08789 with merge a5f1195d10efd5379072c89e24f0b56939fb4644...

bors · 2017-12-20T01:18:13Z

💔 Test failed - status-travis

alexcrichton · 2017-12-20T03:14:37Z

@bors: retry

wedged download?

bors · 2017-12-20T06:38:21Z

⌛ Testing commit bf08789 with merge 6dbf0ba...

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 · 2017-12-20T09:13:50Z

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

rust-highfive assigned sfackler Nov 24, 2017

SimonSapin mentioned this pull request Nov 24, 2017

Implement and forward more formatting traits for slices and arrays #46215

Closed

Make fmt::DebugList and friends forward formatting parameters

e0e7ac3

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

SimonSapin force-pushed the fmt-debuglist-flags branch from 793b55c to e0e7ac3 Compare November 24, 2017 13:17

kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Nov 24, 2017

bluss reviewed Nov 25, 2017

View reviewed changes

SimonSapin mentioned this pull request Nov 26, 2017

Hexadecimal integers with fmt::Debug, including within larger types rust-lang/rfcs#2226

Merged

Keep access to private Formatter fields in Formatter methods

bf08789

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 Nov 28, 2017

kennytm added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 6, 2017

rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Dec 19, 2017

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

SimonSapin deleted the fmt-debuglist-flags branch February 16, 2018 21:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

SimonSapin commented Nov 24, 2017 •

edited

Loading

rust-highfive commented Nov 24, 2017

bluss Nov 25, 2017

SimonSapin Nov 25, 2017

bluss Nov 25, 2017

SimonSapin Nov 26, 2017

SimonSapin Nov 27, 2017

bluss Nov 27, 2017

sfackler commented Nov 28, 2017

SimonSapin commented Nov 28, 2017

sfackler commented Nov 28, 2017

rfcbot commented Nov 28, 2017 •

edited by aturon

Loading

alexcrichton commented Nov 29, 2017

kennytm commented Dec 6, 2017

kennytm commented Dec 13, 2017

rfcbot commented Dec 19, 2017

alexcrichton commented Dec 19, 2017

bors commented Dec 19, 2017

bors commented Dec 20, 2017

bors commented Dec 20, 2017

alexcrichton commented Dec 20, 2017

bors commented Dec 20, 2017

bors commented Dec 20, 2017

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

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

Conversation

SimonSapin commented Nov 24, 2017 • edited Loading

rust-highfive commented Nov 24, 2017

bluss Nov 25, 2017

Choose a reason for hiding this comment

SimonSapin Nov 25, 2017

Choose a reason for hiding this comment

bluss Nov 25, 2017

Choose a reason for hiding this comment

SimonSapin Nov 26, 2017

Choose a reason for hiding this comment

SimonSapin Nov 27, 2017

Choose a reason for hiding this comment

bluss Nov 27, 2017

Choose a reason for hiding this comment

sfackler commented Nov 28, 2017

SimonSapin commented Nov 28, 2017

sfackler commented Nov 28, 2017

rfcbot commented Nov 28, 2017 • edited by aturon Loading

alexcrichton commented Nov 29, 2017

kennytm commented Dec 6, 2017

kennytm commented Dec 13, 2017

rfcbot commented Dec 19, 2017

alexcrichton commented Dec 19, 2017

bors commented Dec 19, 2017

bors commented Dec 20, 2017

bors commented Dec 20, 2017

alexcrichton commented Dec 20, 2017

bors commented Dec 20, 2017

bors commented Dec 20, 2017

SimonSapin commented Nov 24, 2017 •

edited

Loading

rfcbot commented Nov 28, 2017 •

edited by aturon

Loading