Format numbers #72
Format numbers #72
Conversation
| @@ -34,6 +34,7 @@ function Polyglot(options) { | |||
| this.phrases = {}; | |||
| this.extend(opts.phrases || {}); | |||
| this.currentLocale = opts.locale || 'en'; | |||
| this.numberFormat = opts.numberFormat || { format: String }; | |||
There was a problem hiding this comment.
i feel like it'd be better to just accept a function here, and let the user handle preserving the receiver if desired. ie, the default here should maybe just be String?
There was a problem hiding this comment.
Aha! I didn't realize Intl.NumberFormat.prototype.format is a getter that returns a function. (I thought it was a function.) So this is doable.
Still, I hesitate. NumberFormat is the name of a class, so naming an option numberFormat implies that the user should pass an instance of NumberFormat. It's a bit janky for the calling convention to be, new Polyglot({ locale: 'en', numberFormat: new Intl.NumberFormat('en').format }) -- the final .format seems like a typo.
I see three options that are better than what's in there now. Please choose one:
- Accept
numberFormatas a duck-typedIntl.NumberFormat, and throw an error iftypeof options.numberFormat !== 'object' || typeof options.numberFormat.format !== 'function' - Accept
numberFormatas a function, and throw an error iftypeof options.numberFormat !== 'function' - Accept
numberFormatas either: if it's a function, wrap it in an object.
My vote's for 1, because it adheres to modern JS convention. I see how 2 is better in AirBnB's case today; but when AirBnB upgrades away from Node 0.10 and IE10 fades away, the "function" convention will feel outdated. I dislike 3 the most, because ick :).
Aaanyway. I'll happily implement any!
There was a problem hiding this comment.
I'm not sure I buy that any "function convention" would be outdated - that's the way most JS APIs work, and are moving to. I also do see how if you see the name "numberFormat" and are familiar with Intl (a supreme minority today, but likely the majority long term) you might expect it to be related to "NumberFormat".
I do think that a requirement for someone that just has a function to wrap it in an object isn't acceptable, and feels way too Java-y.
I'm not sure what the best approach here is, but "just pass a plain function" is undeniably the most idiomatic API style in JS. Option 2 is what I'd expect anywhere, option 3 might be a good compromise - I'll have think about it more.
|
|
||
| var replacement = options[argument]; | ||
| if (typeof replacement === 'number') { | ||
| replacement = numberFormat.format(replacement); |
There was a problem hiding this comment.
thus here would be just numberFormat(replacement)
| @@ -6,7 +6,7 @@ | |||
| "scripts": { | |||
| "pretest": "npm run --silent lint", | |||
| "test": "npm run --silent tests-only", | |||
| "tests-only": "mocha test/*.js --reporter spec", | |||
| "tests-only": "NODE_ICU_DATA=node_modules/full-icu mocha test/*.js --reporter spec", | |||
There was a problem hiding this comment.
this looks like it needs a devDependency so it can work?
There was a problem hiding this comment.
Whoops! This is a relic. No, we don't need the devDependency because we don't rely on Intl.NumberFormat for unit tests any more. I'll nix it.
|
Heh, I agree with everything. I think the dilemma is that Intl.NumberFormat isn't idiomatic JS :). |
ljharb
force-pushed
the
format-numbers
branch
2 times, most recently
from
bf3acdc to
e54353b
Compare
|
Any progress? Really needful feature! |
Second attempt -- this should address all issues in #69.