lsd
lsd copied to clipboard
Support for bytes separated by thousand with comma
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
Codecov Report
Merging #569 (6f1b202) into master (676d5ab) will decrease coverage by
0.52%. The diff coverage is89.65%.
@@ 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 dataPowered by Codecov. Last update 676d5ab...6f1b202. Read the comment docs.
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.
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
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/
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?
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
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.
Hi @arkadiuszbielewicz, could you please fix the CI error, and rebase onto the master, also squash your commits for a git clean history.