-
-
Notifications
You must be signed in to change notification settings - Fork 437
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
Add IEC and SI units/unit prefixes #826
base: master
Are you sure you want to change the base?
Conversation
The currents unit tests in |
Shouldn't the |
Yes they should, I'll fix this when I get back to my computer. |
The constants use uppercase to comply with rust's naming scheme, but the actual strings are in the correct mixed case. |
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.
thanks for the contribution, and sorry for the late reply, I have read your code, and it looks good to me mostly.
but I would like to mention that we are aligning to the GNU ls, and it has a --si
flag to do this, can you change the flag to align with the GNU ls?
const MB: u64 = 1024_u64.pow(2); | ||
const GB: u64 = 1024_u64.pow(3); | ||
const TB: u64 = 1024_u64.pow(4); | ||
const KIB: u64 = 1024; |
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.
can you add a comment here to record that we use KIB
instead of KiB
for rust naming conventions?
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: gryffyn The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: gryffyn <[email protected]>
I've added the --si flag and the comment about the Rust naming. |
Hi @gryffyn so please help with the following:
|
Forgot about the docs, will update them and fix the merge conflict. Which |
I prefer this one, so that |
The issue then is that there's currently three "modes". |
I have checked the GNU ls, it is not adding the also, we can drop the space between number and unit to make size as only one block
|
To what extent do you want to just replicate ls and how opinionated do you want to be? Because I would argue that a "modern" ls replacement should go the same way as various DEs and distros and adopt the the IEC recommendations entirely. Making binary rather than metric default, as it is in ls, is fine by me, but I think the use of binary units should be then clearly flagged to the user by using the binary prefixes. The --si option can then give the SI prefixes. But I think mixing the SI prefixes with the binary unit is a bad idea, it goes against both modern convention and international agreement, leads to further confusion on the topic, and is only really still used by Windows. I also, as a scientist, very much support the current style of the size in lsd, with a space between value and unit and the additional B, as it is the correct way to do it per the SI. Personally, I also find it much more aesthetically pleasing. I would view this as a positive upgrade over traditional ls. |
hi @matterhorn103, thanks for your options. every opensource project needs a principle to drive the development, as for lsd, we try to align with the GNU ls, that's why I was discussing that. as this could have some more discussion, I am deferring this PR to the Next release, so we can do more discussion. as for the current output format it has worked for years, I am ok to keep it unchanged. but we have to involve a break change for the prefix and unit, as for keeping GNU ls compatibility, I prefer the iec prefix and unit by default, and what is your idea? @gryffyn |
I understand that aligning with GNU ls is a primary goal :) I like your suggestion @zwpaper: that the lsd default changes to become the same as with a new --iec flag, with both IEC prefixes and units, and a new --si flag gives SI prefixes and units. That's the simplest solution that is both "correct" and aligns to ls. (Aligns in that it gives the same numbers, anyway – the fact that the formatting is slightly different seems fine to me, since nicer/better formatting is basically what lsd is for, right?) To follow the example of @gryffyn A 4,000,000 byte would thus be represented as:
|
I agree with @matterhorn103 and @zwpaper, using IEC units and prefixes as default and having SI units/prefixes behind a flag would work well. I'll rework this PR soon to reflect that. |
lsd
currently uses IEC units, but SI prefixes. This PR adds two values to the--size
flag and the associated config file entry.--size iec
uses IEC units and IEC prefixes (KiB, MiB, etc.)--size si
uses SI units and SI prefixes (KB, MB, etc.)A 4,000,000 byte would thus be represented as:
3.8 MB
with--size default
3.8 MiB
with--size iec
4.0 MB
with--size si
TODO
cargo fmt