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

Support for bytes separated by thousand with comma #569

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

Conversation

arkadiuszbielewicz
Copy link
Contributor

@arkadiuszbielewicz arkadiuszbielewicz commented Oct 15, 2021

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

  • Use cargo fmt
  • Add necessary tests
  • Add changelog entry

@codecov-commenter
Copy link

codecov-commenter commented Oct 15, 2021

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%) ⬆️
src/meta/size.rs 94.35% <86.95%> (+0.10%) ⬆️
src/flags/size.rs 94.66% <100.00%> (-0.99%) ⬇️
src/color.rs 51.19% <0.00%> (-5.36%) ⬇️
src/flags/recursion.rs 88.13% <0.00%> (-3.39%) ⬇️
src/config_file.rs 69.31% <0.00%> (-2.28%) ⬇️
src/meta/name.rs 92.44% <0.00%> (-1.34%) ⬇️
src/color/theme.rs 53.75% <0.00%> (-1.25%) ⬇️
src/flags/blocks.rs 93.01% <0.00%> (-0.88%) ⬇️
... 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.

@meain
Copy link
Member

meain commented Oct 18, 2021

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 ogham/exa#554.

@zwpaper
Copy link
Member

zwpaper commented Oct 18, 2021

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

@meain
Copy link
Member

meain commented Oct 18, 2021

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/

@arkadiuszbielewicz
Copy link
Contributor Author

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?

@meain
Copy link
Member

meain commented Oct 20, 2021

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

@arkadiuszbielewicz
Copy link
Contributor Author

arkadiuszbielewicz commented Nov 4, 2021

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.

…ery useful | Potential fix for unix musl
@zwpaper
Copy link
Member

zwpaper commented Nov 5, 2021

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

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.

4 participants