-
Notifications
You must be signed in to change notification settings - Fork 33
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
feat: display type #76
Conversation
b21a0f7
to
fd03e01
Compare
src/lib.rs
Outdated
f.pad(&to_string_format(self.0, Format::IEC)) | ||
f.pad(&self.display().to_string()) |
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.
be nice to get rid of this allocation
fd03e01
to
190d56c
Compare
src/display.rs
Outdated
let unit_prefixes = match self.format { | ||
Format::Iec | Format::Short => crate::UNITS_IEC.as_bytes(), | ||
Format::Si => crate::UNITS_SI.as_bytes(), | ||
}; |
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.
Short
is currently assumed to use IEC (binary) units due to the motivating use case of sort -h
. WDYT about that?
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.
How about combination:
enum ByteUnit {
Iec,
Si,
}
enum ByteFormat {
/// Human readable output.
Human(ByteUnit),
/// For `sort -h` and friends.
Short(ByteUnit),
}
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.
how we repr this internal isn't very important since we're exposing methods on Display
i've added a note on the iec_short
method about compat with sort -h
d1877f8
to
0484052
Compare
0484052
to
5f599dc
Compare
The added display benchmark shows that the additional fast path branch is worth it:
Table sorted by median ascending (lower is beter) |
a0319cc
to
277aefe
Compare
if f.width().is_none() && f.precision().is_none() { | ||
// allocation-free fast path for when no formatting options are specified | ||
write!(f, "{display}") | ||
} else { | ||
f.pad(&display.to_string()) | ||
} |
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 fast path trick was taken from std::fmt::Formatter::pad
https://doc.rust-lang.org/1.84.0/src/core/fmt/mod.rs.html#1438-1442
277aefe
to
61c2d9c
Compare
61c2d9c
to
4725e58
Compare
bump @MrCroxx for review i'm keen to get v2 prepped after this |
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.
LGTM
Thank you for your contribution. The code is awesome. 🤩
I have no comments on the code. But may I ask where does the "short" format come from, which standard or similar library?
we discussed a bit here #14 but i'm open to changing the name "short" to something else |
waiting for #71
similar to #32