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 --number-separator CLI flag #892

Merged
merged 6 commits into from
Dec 12, 2022
Merged

Add --number-separator CLI flag #892

merged 6 commits into from
Dec 12, 2022

Conversation

o2sh
Copy link
Owner

@o2sh o2sh commented Dec 11, 2022

refers to #706

image
image

Impacted by the CLI flag:

  • Number of branches and number of tags
  • Number of dependencies
  • Number of commits per author
  • Number of commits
  • LOC
  • Number of files

@o2sh o2sh requested a review from Byron as a code owner December 11, 2022 16:34
@o2sh o2sh removed the request for review from Byron December 11, 2022 16:34
@o2sh o2sh marked this pull request as draft December 11, 2022 16:42
@o2sh o2sh changed the title Add format-numbers CLI flag Add --format-numbers CLI flag Dec 11, 2022
@spenserblack
Copy link
Collaborator

Some things to keep in mind:

So, if --format-numbers defines the thousands separator, it can be a breaking change if we need to rename that arg to accommodate other options. We could

  • rename --format-numbers to --number-separator or something, or
  • change --format-numbers totake names of formats (we might want to look into what other tools call various number formats).

We could even take a number format like Excel does, #,###.00, so that users can define any placement of separators, and what types of separators to use, without having to raise future issues. That might be overengineering for our needs, though 😆

@o2sh o2sh changed the title Add --format-numbers CLI flag Add --number-separator CLI flag Dec 12, 2022
@o2sh
Copy link
Owner Author

o2sh commented Dec 12, 2022

You're right, my first implementation actually took a localization as an input, but I reverted in 7c6cbba when I found out that num_crate did not support floating numbers as it would have been misleading to users if the decimal character wasn't affected.

I renamed it to --number-separator after your suggestion.

tests/snapshots/repo__repo.snap Outdated Show resolved Hide resolved
@@ -49,7 +49,7 @@ expression: info
"lastChange": "22 years ago",
"contributors": 4,
"repoUrl": "https://github.com/user/repo.git",
"numberOfCommits": "4",
"numberOfCommits": 4,
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes more sense for this value to be a usize 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that makes sense to me, too.

@o2sh o2sh merged commit 3cff2d2 into main Dec 12, 2022
@o2sh o2sh deleted the feat/format-numbers branch December 12, 2022 21:13
@o2sh o2sh mentioned this pull request Dec 14, 2022
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.

None yet

2 participants