-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Improve and fix filesize formatting/display #14397
Conversation
a8a7372
to
2c74c67
Compare
2c74c67
to
f209617
Compare
Cargo.lock
Outdated
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.
Holy moly is that removing many dependencies!
} | ||
|
||
// Safety: all the characters in `buf` are valid UTF-8. | ||
let fract = unsafe { std::str::from_utf8_unchecked(&buf[..i]) }; |
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.
The debug_assert
above should catch potential errors that could make this invalid UTF-8. I could change this to std::str::from_utf8
paired with an expect
instead if that is better.
69bb18d
to
454bda6
Compare
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.
In your changes to the PR description you talk about a space
option. The code doesn't reflect that yet, so maybe you miss a push here. In teneral I kind of lean in the direction that we should for our Display
output not include a space as this corresponds to the literal instead of being typographically correct.
I haven't vetted the nasty formatting algorithm to minimize the float errors with base 10 yet. If your algo is tested well I am probably fine with that debug_assert!
Generally I like the changes in the config layout as is (don't care for the space) and happy about the more Nushelly structure around the config internals. (and good riddance byte_unit
's transitive dependencies(
Sorry for the confusion, I added the If necessary, we can always add the |
Description
This PR cleans up the code surrounding formatting and displaying file sizes.
byte_unit
crate we use for file size units displays kilobytes asKB
, which is not the SI or ISO/IEC standard. Rather it should bekB
, so this fixes SI Kilobyte should be written as "kB", not "KB" #8872. On some systems,KB
actually meansKiB
, so this avoids any potential confusion.byte_unit
crate, when displaying file sizes, casts integers to floats which will lose precision for large file sizes. This PR adds a customDisplay
implementation forFilesize
that can give an exact string representation of aFilesize
for metric/SI units.byte_unit
crate which brought in several other dependencies.Additionally, this PR makes some changes to the config for filesize formatting (
$env.config.filesize
).metric
andformat
options. If a metric (SI) unit was set informat
, butmetric
was set to false, then themetric
option would take precedence and convertformat
to the corresponding binary unit (or vice versa). E.g.,{ format: kB, metric: false }
=>KiB
. Instead, this PR adds theunit
option to replace theformat
andmetric
options.unit
can be set to a fixed file size unit likekB
orKiB
, or it can be set to one of the special options:binary
ormetric
. These options tells nushell to format file sizes using an appropriately scaled metric or binary unit (examples below).precision
option to the filesize config. It determines how many digits to show after the decimal point. If set to null, then everything after the decimal point is shown.{ unit: metric, precision: 1 }
.User-Facing Changes
table
,into string
,to text
, etc.).format filesize
is now case sensitive. An error with the valid units is shown if the case does not match.$env.config.filesize.format
and$env.config.filesize.metric
are deprecated and replaced by$env.config.filesize.unit
.$env.config.filesize.precision
option was added.Tests + Formatting
Mostly updated test expected outputs.
After Submitting
This PR does not change the way NUON serializes file sizes, because that would require changing the nu parser to be able to losslessly decode the new, exact string representation introduced in this PR.
Similarly, this PR also does not change the file size parsing in any way. Although the file size units provided to
format filesize
or the filesize config are now case-sensitive, the same is not yet true for file size literals in nushell code.