Skip to content
This repository was archived by the owner on Dec 8, 2023. It is now read-only.

feat: add thousands separator#18

Merged
morgante merged 5 commits intogetgrit:mainfrom
vlopezferrando:thousands-separator
Sep 23, 2023
Merged

feat: add thousands separator#18
morgante merged 5 commits intogetgrit:mainfrom
vlopezferrando:thousands-separator

Conversation

@vlopezferrando
Copy link
Contributor

@vlopezferrando vlopezferrando commented Sep 18, 2023

Add thousands separator (1_000_000) to numbers (ints and floats, positive or negative).

Limitations:

  • A max number of 4 underscores are added (the implementation has a copy-pasted regex, which may be improved with some kind of recursive logic).

Copy link
Contributor

@morgante morgante left a comment

Choose a reason for hiding this comment

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

I'm pretty sure we could do this generically without recursion, using either a more complex regex or a split of some sort.

@morgante morgante changed the title Thousands separator feat: add thousands separator Sep 18, 2023
Comment on lines +12 to +13
`$number` where {
$number <: or { integer(), float() },
Copy link
Contributor

Choose a reason for hiding this comment

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

This is another example of the anti-pattern (maybe we should put this in the docs). If a variable is immediately restricted after assignment, start with the restriction.

Suggested change
`$number` where {
$number <: or { integer(), float() },
or { integer(), float() } as number where {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@vlopezferrando
Copy link
Contributor Author

Would it be better if the pattern just added one underscore?

1000 -> 1_000
1000000 -> 1000_000
1000_000 -> 1_000_000

Then, if applied multiple times, it would yield the right output no matter how long the number is.

@morgante morgante merged commit 040b698 into getgrit:main Sep 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants