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 support for displaying file sizes in bytes with a separator #1112

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fgimian
Copy link

@fgimian fgimian commented Dec 29, 2024

This is a redo of the excellent work by @areq212 on PR #569 for supporting thousand separators when displaying bytes.


TODO

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

@fgimian fgimian requested a review from zwpaper as a code owner December 29, 2024 10:30
@fgimian
Copy link
Author

fgimian commented Dec 29, 2024

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 fgimian force-pushed the support-bytes-with-separator branch 4 times, most recently from a3b3dea to 257b2f5 Compare December 29, 2024 11:25
@fgimian fgimian marked this pull request as draft December 29, 2024 11:42
@fgimian
Copy link
Author

fgimian commented Dec 29, 2024

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 fgimian force-pushed the support-bytes-with-separator branch 4 times, most recently from ef88612 to 9e428d7 Compare December 29, 2024 12:37
@fgimian fgimian marked this pull request as ready for review December 29, 2024 13:30
@fgimian
Copy link
Author

fgimian commented Dec 29, 2024

I think this is now ready for review 😊

@fgimian fgimian force-pushed the support-bytes-with-separator branch 2 times, most recently from 1b18af2 to 24bab3c Compare December 30, 2024 00:06
@fgimian fgimian force-pushed the support-bytes-with-separator branch from 24bab3c to 481844b Compare December 30, 2024 00:29
@fgimian
Copy link
Author

fgimian commented Dec 31, 2024

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

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.

1 participant