-
Notifications
You must be signed in to change notification settings - Fork 104
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
Normative: Integrate Intl NumberFormat v3 into ECMA-402 #753
Conversation
@gibson042 or @ryzokuken can you suggest how to fix the following ecmarkup error:
causes errors such as
|
@sffc https://tc39.es/ecmarkup/#variable-declarations - 1. Let _n1_ and _e1_ each be an integer and _r1_ a mathematical value, with <emu-eqn>_r1_ = ToRawPrecisionFn(_n1_, _e1_, _p_)</emu-eqn>, such that <emu-eqn>_r1_ ≤ _x_</emu-eqn> and _r1_ is maximized.
- 1. Let _n2_ and _e2_ each be an integer and _r2_ a mathematical value, with <emu-eqn>_r2_ = ToRawPrecisionFn(_n2_, _e2_, _p_)</emu-eqn>, such that <emu-eqn>_r2_ ≥ _x_</emu-eqn> and _r2_ is minimized.
+ 1. [declared="n1,e1,r1"] Let _n1_ and _e1_ each be an integer and _r1_ a mathematical value, with <emu-eqn>_r1_ = ToRawPrecisionFn(_n1_, _e1_, _p_)</emu-eqn>, such that <emu-eqn>_r1_ ≤ _x_</emu-eqn> and _r1_ is maximized.
+ 1. [declared="n2,e2,r2"] Let _n2_ and _e2_ each be an integer and _r2_ a mathematical value, with <emu-eqn>_r2_ = ToRawPrecisionFn(_n2_, _e2_, _p_)</emu-eqn>, such that <emu-eqn>_r2_ ≥ _x_</emu-eqn> and _r2_ is minimized. |
Reviewers: please double-check that the diff in the PR corresponds to the diff in the proposal. I will also double-check this but it's good to have extra verification of that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still need to review the bulk of this PR in numberformat.html, but I have some comments on the changes that touch pluralrules.html.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall the spec looks great, I left a few comments inline but most of them are minor and some could even be addressed later on.
</emu-clause> | ||
|
||
<emu-clause id="sec-formatnumericrangetoparts" aoid="FormatNumericRangeToParts"> | ||
<h1>FormatNumericRangeToParts ( _numberFormat_, _x_, _y_ )</h1> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we switch to the structured headers syntax? It doesn't necessarily need to be done before this is merged, but let's do it sooner rather than later.
This applies to every new AO, I suppose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Acknowledged. I wrote most of this spec before structured headers were common in ECMA-402. In an effort to get this merged, I would rather make only small changes that are unlikely to require extensive further review. However, this sounds like a great editorial task to be done after merging this PR. :)
spec/numberformat.html
Outdated
<dd>It populates the internal slots of _intlObj_ that affect locale-independent number rounding (see <emu-xref href="#sec-formatnumberstring"></emu-xref>).</dd> | ||
</dl> | ||
<emu-clause id="sec-setnfdigitoptions" aoid="SetNumberFormatDigitOptions"> | ||
<h1>SetNumberFormatDigitOptions ( _intlObj_, _options_, _mnfdDefault_, _mxfdDefault_, _notation_ )</h1> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an important reason to move away from the structured headers syntax here? Since the new description doesn't include the "type" information unlike the previous style, we're just losing that information...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, it looks like this change should be reverted to keep the current SetNumberFormatDigitOptions <h1>
and <dl>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May have gotten lost as part of the merge. Will fix.
1. If _roundingIncrement_ is not in « 1, 2, 5, 10, 20, 25, 50, 100, 200, 250, 500, 1000, 2000, 2500, 5000 », throw a *RangeError* exception. | ||
1. Let _roundingMode_ be ? GetOption(_options_, *"roundingMode"*, ~string~, « *"ceil"*, *"floor"*, *"expand"*, *"trunc"*, *"halfCeil"*, *"halfFloor"*, *"halfExpand"*, *"halfTrunc"*, *"halfEven"* », *"halfExpand"*). | ||
1. Let _trailingZeroDisplay_ be ? GetOption(_options_, *"trailingZeroDisplay"*, ~string~, « *"auto"*, *"stripIfInteger"* », *"auto"*). | ||
1. NOTE: All fields required by SetNumberFormatDigitOptions have now been read from _options_. The remainder of this AO interprets the options and may throw exceptions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't feel strongly either way, but not entirely convinced that this NOTE is necessary. That said, if it helps implementers enough to justify keeping it in, why not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this line as part of resolving an editorial issue back on the spec repository where I was reading options in a bunch of different places. I think the NOTE could be useful for future changes in order to ensure that we are consistent in the point at which we read the options.
spec/numberformat.html
Outdated
<emu-clause id="sec-collapsenumberrange" aoid="CollapseNumberRange"> | ||
<h1>CollapseNumberRange ( _result_ )</h1> | ||
<p> | ||
The CollapseNumberRange abstract operation modifies _result_, which must be a List of Record values as described in PartitionNumberRangePattern, by removing redundant information, and returns the resulting List. The algorithm is implementation dependent, but it must not introduce ambiguity: the resulting list after modification should be unique for all pairs _x_ and _y_. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's difficult to understand this sentence without context, since one might not know x
and y
, could it be worded differently? Like I think "for all pairs start
and end
" already sounds easier to wrap your head around.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
spec/numberformat.html
Outdated
The FormatApproximately abstract operation modifies _result_, which must be a List of Record values as described in PartitionNumberPattern, by adding a new Record for the approximately sign, which may depend on _numberFormat_ (which must be an object initialized as NumberFormat). The following steps are taken: | ||
</p> | ||
<emu-alg> | ||
1. Let _i_ be an index into _result_, determined by an implementation-defined algorithm based on _numberFormat_ and _result_. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: could we add a bit more info to guide implementations here at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a sentence
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @ryzokuken , and have a few nits/suggestions but everything overall looks good.
spec/numberformat.html
Outdated
1. Else, | ||
1. Set _intlObj_.[[RoundingType]] to ~fractionDigits~. | ||
1. Set _intlObj_.[[RoundingType]] to ~morePrecision~. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these steps can also be reordered for clarity by setting [[RoundingType]] only after every step that sets [[{Minimum,Maximum}{Fraction,Significant}Digits]]:
1. If _needSd_ is *true*, then
…set _intlObj_.[[{Minimum,Maximum}SignificantDigits]] based on {_mnsd_,_mxsd_} or {1,21}…
1. If _needfd_ is *true*, then
…set _intlObj_.[[{Minimum,Maximum}FractionDigits]] based on {_mnfd_,_mxfd_} or {_mnfdDefault_,_mxfdDefault_}…
1. If _needSd_ is *false* and _needFd_ is *false*, then
1. Set _intlObj_.[[MinimumFractionDigits]] to 0.
1. Set _intlObj_.[[MaximumFractionDigits]] to 0.
1. Set _intlObj_.[[MinimumSignificantDigits]] to 1.
1. Set _intlObj_.[[MaximumSignificantDigits]] to 2.
1. Set _intlObj_.[[RoundingType]] to ~morePrecision~.
1. Else,
1. If _roundingPriority_ is *"morePrecision"*, set _intlObj_.[[RoundingType]] to ~morePrecision~.
1. Else if _roundingPriority_ is *"lessPrecision"*, set _intlObj_.[[RoundingType]] to ~lessPrecision~.
1. Else if _hasSd_ is *true*, set _intlObj_.[[RoundingType]] to ~significantDigits~.
1. Else, set _intlObj_.[[RoundingType]] to ~fractionDigits~.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
1. If _nf_.[[RoundingType]] is ~morePrecision~, then | ||
1. Perform ! CreateDataPropertyOrThrow(_options_, *"roundingPriority"*, *"morePrecision"*). | ||
1. Else if _nf_.[[RoundingType]] is ~lessPrecision~, then | ||
1. Perform ! CreateDataPropertyOrThrow(_options_, *"roundingPriority"*, *"lessPrecision"*). | ||
1. Else, | ||
1. Perform ! CreateDataPropertyOrThrow(_options_, *"roundingPriority"*, *"auto"*). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make more sense to just introduce a [[RoundingPriority]] slot and avoid the potential of this recovery logic falling out of sync in the future (and also the minor weirdness of "roundingPriority" being required to appear as the last property of the object returned from resolvedOptions()
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would rather not make this big of a change at this point. Also, this logic reflects that RoundingPriority is now a computed property: if compact notation is used, it gets computed as "morePrecision", which is needed for round-tripping.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make more sense to just introduce a [[RoundingPriority]] slot and avoid the potential of this recovery logic falling out of sync in the future (and also the minor weirdness of "roundingPriority" being required to appear as the last property of the object returned from
resolvedOptions()
)?
Would rather not make this big of a change at this point. Also, this logic reflects that RoundingPriority is now a computed property: if compact notation is used, it gets computed as "morePrecision", which is needed for round-tripping.
@ryzokuken Thoughts on the above? I can live with it, albeit unhappily.
spec/numberformat.html
Outdated
</tr> | ||
</tbody> | ||
</table> | ||
<emu-note>The examples are illustrative of the unique behaviour of each option. ➕ means "resolves toward positive infinity"; ➖ means "resolves toward negative infinity".</emu-note> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I appreciate the information, but I wonder if and collide too much with actual numeric signs. Did you consider arrows instead?
- ↥ and ↧
- ⤒ and ⤓
- ⬆ and ⬇️
- ⭱ and ⭳
- ⮉ and ⮋
- ⇧ and ⇩
- etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arrow emoji SGTM
Co-authored-by: Richard Gibson <richard.gibson@gmail.com>
1. If _nf_.[[RoundingType]] is ~morePrecision~, then | ||
1. Perform ! CreateDataPropertyOrThrow(_options_, *"roundingPriority"*, *"morePrecision"*). | ||
1. Else if _nf_.[[RoundingType]] is ~lessPrecision~, then | ||
1. Perform ! CreateDataPropertyOrThrow(_options_, *"roundingPriority"*, *"lessPrecision"*). | ||
1. Else, | ||
1. Perform ! CreateDataPropertyOrThrow(_options_, *"roundingPriority"*, *"auto"*). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make more sense to just introduce a [[RoundingPriority]] slot and avoid the potential of this recovery logic falling out of sync in the future (and also the minor weirdness of "roundingPriority" being required to appear as the last property of the object returned from
resolvedOptions()
)?
Would rather not make this big of a change at this point. Also, this logic reflects that RoundingPriority is now a computed property: if compact notation is used, it gets computed as "morePrecision", which is needed for round-tripping.
@ryzokuken Thoughts on the above? I can live with it, albeit unhappily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few final nits, with the one applying to PartitionNumberRangePattern being a blocking concern (but easy to resolve).
Co-authored-by: Richard Gibson <richard.gibson@gmail.com>
Thanks for the suggestions; they all looked good to me |
@gibson042 this could be addressed by a follow-up editorial change, right? I want to kick off the opt-out ASAP and then we could clean up the working draft. |
It would only be editorial if we commit to the awkward property ordering: …, The property ordering is already awkward, which is basically why I can live this, but my preference is still to avoid making things worse. |
https://github.com/tc39/proposal-intl-numberformat-v3