Skip to content
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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

gryffyn
Copy link

@gryffyn gryffyn commented Mar 26, 2023

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

  • Use cargo fmt
  • Add necessary tests
  • Add changelog entry
  • Update default config/theme in README (if applicable)
  • Update man page at lsd/doc/lsd.md (if applicable)

@gryffyn
Copy link
Author

gryffyn commented Mar 26, 2023

The currents unit tests in src/meta/size.rs use IEC units (1024) for checking size.value_string() and I'm unsure whether I need to write tests that use SI (1000) for checking the value. If not, adding tests can be marked as done.

@frankebel
Copy link

frankebel commented Aug 28, 2023

Shouldn't the i in binary prefixes be lower case? So KiB, MiB, ... instead of KIB, MIB, ... Wikipedia

@gryffyn
Copy link
Author

gryffyn commented Sep 1, 2023

Yes they should, I'll fix this when I get back to my computer.

@gryffyn
Copy link
Author

gryffyn commented Sep 9, 2023

The constants use uppercase to comply with rust's naming scheme, but the actual strings are in the correct mixed case.

Copy link
Member

@zwpaper zwpaper left a 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?

ref: https://man7.org/linux/man-pages/man1/ls.1.html

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;
Copy link
Member

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?

@muniu-bot
Copy link

muniu-bot bot commented Sep 12, 2023

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Signed-off-by: gryffyn <me@gryffyn.io>
@gryffyn
Copy link
Author

gryffyn commented Sep 12, 2023

I've added the --si flag and the comment about the Rust naming.

@zwpaper
Copy link
Member

zwpaper commented Sep 12, 2023

Hi @gryffyn --si is enough, and it should not be used with the --size flag, --size should be able to cowork with the --si flag.

so please help with the following:

  1. revert the --size changes
  2. add docs for --si
  3. fix merge conflict

@gryffyn
Copy link
Author

gryffyn commented Sep 12, 2023

Forgot about the docs, will update them and fix the merge conflict.

Which --size changes should I revert? Do you mean that--si should be its own flag, and --size should have the options default, short, iec, si, bytes, or that --si should be the only flag that controls toggling SI units and --size should have the options default, short, bytes?

@zwpaper
Copy link
Member

zwpaper commented Sep 13, 2023

that --si should be the only flag that controls toggling SI units and --size should have the options default, short, bytes?

I prefer this one, so that --si could be used with --size

@zwpaper zwpaper added this to the v1.1.0 milestone Sep 13, 2023
@gryffyn
Copy link
Author

gryffyn commented Sep 13, 2023

The issue then is that there's currently three "modes". lsd's default is IEC units and SI prefixes. Adding the --si flag adds the ability to use SI units and prefixes, but this still leaves out IEC units and prefixes. Should I add an --iec flag that does the same thing as --si but for IEC units instead of SI?

@zwpaper
Copy link
Member

zwpaper commented Sep 14, 2023

I have checked the GNU ls, it is not adding the B suffix for both iec and si format, maybe we can learn from it, only use one character to represent the unit for both iec and si, like:

also, we can drop the space between number and unit to make size as only one block

.rw-r-----. root    root      11K Mon Sep 11 16:25:41 2023  kdump.log
.rw-rw-r--. root    root     245G Thu Sep 14 09:40:09 2023  lastlog
.rw-------  root    root        5 Mon Sep 11 17:12:01 2023  maillog

@matterhorn103
Copy link

matterhorn103 commented Nov 6, 2023

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.

@zwpaper zwpaper modified the milestones: v1.1.0, Next Dec 21, 2023
@zwpaper
Copy link
Member

zwpaper commented Dec 21, 2023

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 --si change both.

what is your idea? @gryffyn

@matterhorn103
Copy link

matterhorn103 commented Jan 10, 2024

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:

  • 3.8 MiB by default
  • 3.8 MiB with --iec
  • 4.0 MB with --si

@gryffyn
Copy link
Author

gryffyn commented Jan 10, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants