Implement Intl.PluralRules #4940
Conversation
| * @param {Object} options | ||
| * @param {Number} mnfdDefault | ||
| * @param {Number} mxfdDefault | ||
| */ |
There was a problem hiding this comment.
descriptions for these params might help because I don't know immediately what mxfd means
| return platform.pluralRulesSelect(pluralRules, n); | ||
| }; | ||
|
|
||
| const PluralRules = function (locales = undefined, options = undefined) { |
There was a problem hiding this comment.
= undefined [](start = 45, length = 12)
unsupplied params are undefined by default, we don't need to specify it #ByDesign
There was a problem hiding this comment.
Ah, but test262 is exceptionally finicky about lengths of functions. This allows us to have formal parameters but keep the length as 0, and is done for other functions throughout the file too.
There was a problem hiding this comment.
This raises the point that this is a by-design "anti-pattern" that many may think is unnecessary when reading and updating the code, and probably warrants a comment per-occurrence explaining why. (to wit: "// use = undefined on formals to force the length of the function to be 0, per spec")
jackhorton
force-pushed
the
intl/plural-rules
branch
from
a3d7c3b to
b1d57ec
Compare
jackhorton
force-pushed
the
intl/plural-rules
branch
2 times, most recently
from
f34bdde to
1c21307
Compare
|
|
||
| return JavascriptString::NewWithBuffer(selected, static_cast<charcount_t>(selectedLength), scriptContext); | ||
| #else | ||
| AssertOrFailFastMsg(false, "Intl-WinGlob should not be using PluralRuleKeywords"); |
There was a problem hiding this comment.
PluralRuleKeywords [](start = 69, length = 18)
nit: PluralRulesSelect (?)
|
Overall LGTM. The pre-61 "hack" seems a little questionable. What versions can it work under? (What is the minimum ICU version we support? Are there source code guards for that? Is the hack truly safe? In any case, 61 is (I think?) the main target so it's not a super huge problem. |
|
The hack does suck but I don't know of a better way to handle it aside from dragging every flavor of chakra/core to 61 all at once, kicking and screaming. For some value of "official support," we officially support back to ICU 55, since that's system ICU on Ubuntu 16.04 |
|
@jackhorton since we make assumptions based on minimum version of ICU 55, should we intentionally break the build if we detect an earlier ICU? |
|
The worst thing that can happen is an API that we need not being there, which should be caught at launch (I have seen it happen on Windows at least). I am not aware of critical behavioral differences between versions of the API. |
jackhorton
force-pushed
the
intl/plural-rules
branch
2 times, most recently
from
d604f1d to
54fc55f
Compare
|
Any major changes since the last iteration you'd like a set of eyes on? |
|
Nope, I mainly updated to fix tc39/ecma402#224. I am re-baselining chakrafull right now before merging this. |
jackhorton
force-pushed
the
intl/plural-rules
branch
3 times, most recently
from
59c0f5a to
0c9a60f
Compare
jackhorton
force-pushed
the
intl/plural-rules
branch
from
0c9a60f to
56df095
Compare
After looking into Intl.PluralRules last night, I realized it was really simple and straightforward to implement. This brings us to full Intl API support except for
Intl.NumberFormat.prototype.formatToParts().Fixes #4799