Rewrite <Duration as fmt::Display>::fmt() #25481
Conversation
Implement new formatting behavior that is smarter about the unit it uses
and handles precision.
format!("{}", Duration::new(0, 1_001)) // 1.001µs
format!("{}", Duration::new(0, 1_001_001)) // 1.001ms
format!("{:.6}", Duration::new(0, 1_001_001)) // 1.001001ms
// etc
The default is variable precision up to 3 decimals. No unit higher than
seconds are used.
See rust-lang/rfcs#1121 for more information.
|
(rust_highfive has picked a reviewer for you, use r? to override) |
| // precision in a way we don't want. At such time as it becomes reasonable to support | ||
| // padding without reimplementing fmt::Formatter::with_padding() here, this should be | ||
| // updated to support padding. In preparation for that, we're already writing to a | ||
| // stack buffer. |
There was a problem hiding this comment.
Can you wrap this (and the remaining lines) to 100 characters? Although the style guide says 100 max it's more important to follow the style of the surrounding code.
There was a problem hiding this comment.
I'm not sure what you mean. You're asking me to wrap it to 100, which is already the limit it's wrapped at. Did you mean to ask for a different line length?
FWIW, I typically wrap doc comments at 79 but leave code at 100. I didn't wrap this to 79 because it's not a doc comment.
There was a problem hiding this comment.
Oh oops sorry I meant wrap to 80!
There was a problem hiding this comment.
I just tried wrapping to 80, and the resulting code is significantly harder to read, because a bunch of lines that fit comfortably with 100 now have to be wrapped awkwardly. The code here is indented more than the rest of the module, so they already don't have as much space.
I also don't understand why you think it's important to wrap this to 80 characters. You say it's important to follow the style of the surrounding code, but I don't see any justification at all for that other than the fact that you wrote the surrounding code and you personally prefer 80 characters. If someone else had written this file, I suspect you wouldn't think it's so important. In fact, I know you wouldn't, because prior to 556e76b this file was written for a line limit of 100.
In general, following the style of the surrounding code is a sensible policy when the project does not have a well-defined style. And even when it does, it still makes sense with small changes to preserve the cohesive style. But rust-lang/rust has a (reasonably-)well-defined style, and this is a significant chunk of new functionality (as opposed to individual lines here and there). And since the readability of this code suffers significantly with the shorter line length, I don't think it makes sense to restrict it like that.
|
@alexcrichton Sorry for the delayed response, but I've now responded to your review comments. |
alexcrichton
added
the
T-libs-api
|
I think we may want to hold off on this until a decision on #26818 is reached, as that PR is deleting the display implementation. |
|
It's only deleting the impl under the assumption that this wasn't good to go. If we think we're happy with this Display logic being used for all time, then I'll update my PR to not delete it. |
|
☔ The latest upstream changes (presumably #26818) made this pull request unmergeable. Please resolve the merge conflicts. |
|
So, with #26818 merged, what's the status of this PR? |
|
Good question. I'd really like to see this get merged in. Obviously there's conflicts that need resolving now, but I haven't touched it as there has been no indication as to whether it will be accepted. |
alexcrichton
removed
the
T-libs-api
|
The libs team decided to close this during triage yesterday, more info can be found in my comment on #29146. |
Implement new formatting behavior that is smarter about the unit it uses
and handles precision.
The default is variable precision up to 3 decimals. No unit higher than
seconds are used.
See rust-lang/rfcs#1121 for more information.