-
Notifications
You must be signed in to change notification settings - Fork 2k
Improve and fix filesize formatting/display #14397
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
Conversation
a8a7372 to
2c74c67
Compare
2c74c67 to
f209617
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.
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
sholderbach
left a comment
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 |
The PR that forces this commit nushell/nushell#14397
Description
This PR cleans up the code surrounding formatting and displaying file sizes.
byte_unitcrate 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,KBactually meansKiB, so this avoids any potential confusion.byte_unitcrate, when displaying file sizes, casts integers to floats which will lose precision for large file sizes. This PR adds a customDisplayimplementation forFilesizethat can give an exact string representation of aFilesizefor metric/SI units.byte_unitcrate which brought in several other dependencies.Additionally, this PR makes some changes to the config for filesize formatting (
$env.config.filesize).metricandformatoptions. If a metric (SI) unit was set informat, butmetricwas set to false, then themetricoption would take precedence and convertformatto the corresponding binary unit (or vice versa). E.g.,{ format: kB, metric: false }=>KiB. Instead, this PR adds theunitoption to replace theformatandmetricoptions.unitcan be set to a fixed file size unit likekBorKiB, or it can be set to one of the special options:binaryormetric. These options tells nushell to format file sizes using an appropriately scaled metric or binary unit (examples below).precisionoption 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 filesizeis now case sensitive. An error with the valid units is shown if the case does not match.$env.config.filesize.formatand$env.config.filesize.metricare deprecated and replaced by$env.config.filesize.unit.$env.config.filesize.precisionoption 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 filesizeor the filesize config are now case-sensitive, the same is not yet true for file size literals in nushell code.