lsd icon indicating copy to clipboard operation
lsd copied to clipboard

Add support for displaying file sizes in bytes with a separator

Open fgimian opened this issue 1 year ago • 7 comments

This is a redo of the excellent work by @areq212 on PR https://github.com/lsd-rs/lsd/pull/569 for supporting thousand separators when displaying bytes.


TODO

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

fgimian avatar Dec 29 '24 10:12 fgimian

Investigating why the unit test is failing on Unix (I'm on Windows but will setup WSL or a VM to test and see what's going on).

Edit: I have to admit, I have no idea why this is failing for specific Linux configurations. Any ideas would be more than welcome! 😄

Another Edit: I can see this is failing via cross-compilation which is something I can reproduce so I'll check this out further tomorrow. I'll switch this to draft until everything is resolved, so sorry.

fgimian avatar Dec 29 '24 10:12 fgimian

OK, the problem is simply that the failing environments don't have the required locales pre-installed. As an example, the cross image used (ghcr.io/cross-rs/x86_64-unknown-linux-musl:0.2.5) provides a very limited number of locales out of the box:

root@cd931a9b586d:/# locale -a
C
C.UTF-8
POSIX
root@db27beebc168:/# export LC_ALL=en_US.UTF-8
bash: warning: setlocale: LC_ALL: cannot change locale (en_US.UTF-8): No such file or directory

This can be rectified within the image, but this may be quite tedious to do via CI.

e.g.

# Install the locales package.
apt install locales

# Generate the required locale for testing.
locale-gen en_US.UTF-8

# Observe the locale is now available.
locale -a

Exporting the locale now works:

root@cd931a9b586d:/# export LC_ALL=en_US.UTF-8
root@cd931a9b586d:/#

Sadly all the provided locales define an empty string as a separator which makes it a bit more challenging to set. However, what I may be able to do is detect whether the locale is available and taylor the test accordingly.

fgimian avatar Dec 29 '24 12:12 fgimian

I think this is now ready for review 😊

fgimian avatar Dec 29 '24 13:12 fgimian

Just a little FYI that I've been using this for the last day and it works really well. 😄

fgimian avatar Dec 31 '24 04:12 fgimian

Friendly ping on this PR :) Would love this feature personally.

dconeybe avatar Feb 12 '25 18:02 dconeybe

@zwpaper Kindly let me know if you are interested in this PR. I personally no longer use lsd and would like to delete my fork to clean up my repos.

fgimian avatar May 15 '25 05:05 fgimian

Hi @fgimian, apologies for the off; I was concentrated on a new project for a while, I now have some time to look into LSD at some time.

I have checked the PR, mostly LGTM, but I believe that it would be good to use a new flag for the separator, let me update it and get this merged.

zwpaper avatar May 16 '25 16:05 zwpaper

i am closing this pr in favor of https://github.com/lsd-rs/lsd/pull/1146 since i can not push my update to the fork.

will add @fgimian as co-author in #1146

zwpaper avatar Jul 15 '25 18:07 zwpaper