Skip to content
This repository has been archived by the owner on Apr 11, 2024. It is now read-only.

Locale support to format numbers #210

Closed
pimlie opened this issue Oct 12, 2018 · 6 comments · May be fixed by #216
Closed

Locale support to format numbers #210

pimlie opened this issue Oct 12, 2018 · 6 comments · May be fixed by #216
Labels

Comments

@pimlie
Copy link

pimlie commented Oct 12, 2018

Would you consider accepting a PR to include locale support through an optional dependency like Numeral.js?

The first time I ran a benchmark I thought it was really cool (but strange and doubtful) you measured ops/sec with 3 digit precision. Took a while before I noticed the locale issue...

@jdalton
Copy link
Member

jdalton commented Nov 8, 2018

Hi @pimlie!

Can you explain the locale issue in more detail? What did you see to what did you expect.

@pimlie
Copy link
Author

pimlie commented Nov 8, 2018

default x 91,998 ops/sec ±0.96% (88 runs sampled)
sync x 91,686 ops/sec ±1.01% (86 runs sampled)
async x 85,670 ops/sec ±1.71% (89 runs sampled)
async resolve x 80,033 ops/sec ±0.72% (84 runs sampled)
Fastest is default,sync

Hi @jdalton, thanks for getting back to me on this. The above example uses , as thousand-separators. The , seems to be hardcoded in formatNumber. Like other countries, in the Netherlands we use a dot as thousands separator and a comma as decimal separator. So when I see the above numbers my first instinct is to read them as ~90 ops/sec and not ~90000.

It would be nice if this lib could check the env for which numeric locale it should use (eg LC_NUMERIC) and automatically apply correct thousand separators for that locale. For nl_NL the above example would then read as

default x 91.998 ops/sec ±0,96% (88 runs sampled)

@jdalton
Copy link
Member

jdalton commented Nov 8, 2018

Ah nice! Are you familiar with the Intl APIs?

@pimlie
Copy link
Author

pimlie commented Nov 8, 2018

I have looked at Node's Internationalization support but Number.toLocaleString seems not to be locale-aware without re-compiling Node which seems a bit excessive for 'just' changing the number separators.

@jdalton
Copy link
Member

jdalton commented Nov 8, 2018

I'm up for a PR that enhances Intl when possible, so an optional install and a code path for toLocaleString would be great!

@jdalton
Copy link
Member

jdalton commented Jan 2, 2019

Moved to #216 💪

@jdalton jdalton closed this as completed Jan 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging a pull request may close this issue.

2 participants