lsd icon indicating copy to clipboard operation
lsd copied to clipboard

Support for bytes separated by thousand with comma

Open arkadiuszbielewicz opened this issue 4 years ago • 8 comments

Solution for task #533. Used num-format library as it's really simple to use and seems to be performant. It also allows further extension to support OS locale and grouping not only by thousands but also in Indian style. Used Locale.en as it's most obvious.


TODO

  • [x] Use cargo fmt
  • [x] Add necessary tests
  • [x] Add changelog entry

arkadiuszbielewicz avatar Oct 15 '21 12:10 arkadiuszbielewicz

Codecov Report

Merging #569 (6f1b202) into master (676d5ab) will decrease coverage by 0.52%. The diff coverage is 89.65%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #569      +/-   ##
==========================================
- Coverage   86.40%   85.88%   -0.53%     
==========================================
  Files          37       37              
  Lines        3848     3869      +21     
==========================================
- Hits         3325     3323       -2     
- Misses        523      546      +23     
Impacted Files Coverage Δ
src/app.rs 72.58% <ø> (+3.92%) :arrow_up:
src/meta/size.rs 94.35% <86.95%> (+0.10%) :arrow_up:
src/flags/size.rs 94.66% <100.00%> (-0.99%) :arrow_down:
src/color.rs 51.19% <0.00%> (-5.36%) :arrow_down:
src/flags/recursion.rs 88.13% <0.00%> (-3.39%) :arrow_down:
src/config_file.rs 69.31% <0.00%> (-2.28%) :arrow_down:
src/meta/name.rs 92.44% <0.00%> (-1.34%) :arrow_down:
src/color/theme.rs 53.75% <0.00%> (-1.25%) :arrow_down:
src/flags/blocks.rs 93.01% <0.00%> (-0.88%) :arrow_down:
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 676d5ab...6f1b202. Read the comment docs.

codecov-commenter avatar Oct 15 '21 12:10 codecov-commenter

Hey, thanks for working on this. I remember you mentioning about the cland dependency for picking system locale, but without that I'm not sue how useful this will be. Any thought @zwpaper ?

I found a related issue in exa for this https://github.com/ogham/exa/issues/554.

meain avatar Oct 18 '21 04:10 meain

the issue in exa and lsd shows there seems to be someone who cares about showing the separator, but it may really be strange for the people not using , as thousand separators.

it is also not worth adding the clang dependency for such a minor feature...

I have checked the num-format lib, and found that there is no need for clang for the local feature, the clang is only necessary for auto-detecting the format, how about using the local only, and leave the style to cmd args, configuration file or shell env. @meain

zwpaper avatar Oct 18 '21 06:10 zwpaper

locale is something that should be picked up from the users env as they will have configured that in the OS. Don't think we should ask the user to configure locale setting again specifically for lsd. Maybe we could implement the locale detection of linux and macos alone and skip that in windows. In Windows we can just use 1000 based separator until we find a pure rust lib. There is an alternate crate as well if we are going that route: https://docs.rs/locale/0.2.2/locale/

meain avatar Oct 18 '21 07:10 meain

Exactly as @meain mentioned, my idea was to use only part which doesn't require Clang (it's only used for SystemLocale feature) and with some other code, detect system locale. Should I do it within this PR?

arkadiuszbielewicz avatar Oct 20 '21 05:10 arkadiuszbielewicz

What do you have in mind to get system locale? I think it is better gone in as a single PR just to validate the idea. @arkadiuszbielewicz

meain avatar Oct 20 '21 11:10 meain

Hi, I've checked available options and finally I've decided to use num_format with with-system-locale feature, but only for Unix family. In case of Windows it's disabled with fallback to Locale.en.

arkadiuszbielewicz avatar Nov 04 '21 10:11 arkadiuszbielewicz

Hi @arkadiuszbielewicz, could you please fix the CI error, and rebase onto the master, also squash your commits for a git clean history.

zwpaper avatar Nov 05 '21 03:11 zwpaper