o2sh / onefetch

Command-line Git information tool
https://onefetch.dev
MIT License
9.67k stars 268 forks source link

Format numbers #706

Closed H4ckerxx44 closed 1 year ago

H4ckerxx44 commented 2 years ago

Summary 💡

On larger code bases, the amount of files and the amount of lines of code could become a bit unreadable due to the lacking formatting, my feature request would be to format the lines of code, commits as well as the files datapoint like, 15614 -> 15,614 or 3255204307 -> 3,255,204,307

Motivation 🔦

That's 3 billlion lines of code, quite unreadable grafik

spenserblack commented 2 years ago

Just throwing out another option: what would you think about formats like 3K or 3.3M instead? Similar to the Size field.

H4ckerxx44 commented 2 years ago

Just throwing out another option: what would you think about formats like 3K or 3.3M instead? Similar to the Size field.

While I have nothing against that suggestion, I would not choose this due to it stripping away some "exactness" of the numbers at hand. (Let's ignore the Size in GiB)

o2sh commented 2 years ago

This feature would require to handle multiple formatting options (e.g. which thousands separator to use) and be able to detect the system local of the user (or use a cli flag to ask for the user's preference), num_format seems like a good candidate. Unfortunately the library doesn't seem to be maintained https://github.com/bcmyers/num-format/issues/27

spenserblack commented 2 years ago

TBH I'm not sure how necessary exactness is. This is just my opinion, but once billions of lines of code are reached, I wouldn't care if it's 3 billion, 3 billion and 1 hundred, or 3 billion and 1 thousand, I'd just care about the 3 or 4 most significant digits. If I'm showing off my repo, I'd be saying "look, over 3 billion lines of code!" not "look, 3 billion, 255 million, ..." :laughing: Of course, for other outputs, like JSON, the exact number would be desired, but we wouldn't need to format it.

As @o2sh points out different locales have different thousands separators. And even the placement of the separators can differ (not all cultures historically group numbers into thousands), so any formatting with separators would need to come from a library.

o2sh commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

H4ckerxx44 commented 1 year ago

Well, the activity depends on what you ultimately decide on for this idea. I would just - as said previously - go with the 123,489,645 lines formatting of those numbers.

spenserblack commented 1 year ago

Going through issues: since number format is a personal preference and/or localization issue, I think it should be a config option. So I'm going to suggest that #745 gets implemented before this. After that, I think the most reasonable thing is to offer some predefined named formatters that a user can pick in their config (defaulting to the "raw" format).

o2sh commented 1 year ago

I like this approach. Does this mean we'll need to have a CLI flag --format-numbers in order to have it be part of the Config ? 🤔

spenserblack commented 1 year ago

Depends on how the config is implemented. Preferably, we don't have to duplicate much work.

Honestly, we could make a "config" be purely a list of CLI args (I'm taking inspiration from .yardopts files). If we took this approach, we can continue our current style of making everything a CLI arg. Then we just parse the args that were in the file, then parse the args from the command-line, prioritizing CLI args over "optsfile" args where there's conflicts.

I suppose I'm kind of contradicting myself. I just said to wait until config files are implemented, but now I'm kind of arguing it should be a CLI arg :rofl:

If this approach is worth it, I wonder if a crate already exists for this :thinking:

o2sh commented 1 year ago

FWIW, it seems like num_format came back from the dead https://github.com/bcmyers/num-format/issues/27#issuecomment-1272626722 😅

We should probably use it to format numbers. I'm in favor of adding a CLI flag to allow the user to specify the format with a default to No formatting.

The library comes a with a feature that can automatically detect the OS's locale information but it does not seem like a good idea to use it:

Since this type requires several dependencies (especially on Windows), it is behind a feature flag. To use it, include num-format = { version = "0.4.3", features = ["with-system-locale"] } in your Cargo.toml. Additionally, on Windows (but only on Windows), using SystemLocale requires Clang 3.9 or higher.

spenserblack commented 1 year ago

FWIW, it seems like num_format came back from the dead

Good news! I'm all in favor of using it to format numbers :+1:

o2sh commented 1 year ago

Hi @H4ckerxx44, can you check if #892 resolves your issue? Thanks.

H4ckerxx44 commented 1 year ago

Hi @H4ckerxx44, can you check if #892 resolves your issue? Thanks.

As I am not home right now I can't really check, though it seems like this does exactly what I had in mind.

Thanks for implementing it!