Skip to content
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

Merged
merged 11 commits into from
Apr 6, 2023

Conversation

sffc
Copy link
Contributor

@sffc sffc commented Jan 30, 2023

@sffc
Copy link
Contributor Author

sffc commented Jan 30, 2023

@gibson042 or @ryzokuken can you suggest how to fix the following ecmarkup error:

          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 _r_ be ApplyUnsignedRoundingMode(_x_, _r1_, _r2_, _unsignedRoundingMode_).

causes errors such as

[2023-01-30T17:13:34.352Z] Error: spec/numberformat.html:1263:56: could not find a preceding declaration for "r1"
[2023-01-30T17:13:34.352Z] Error: spec/numberformat.html:1263:62: could not find a preceding declaration for "r2"

@gibson042
Copy link
Contributor

@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.

@sffc sffc marked this pull request as ready for review January 30, 2023 19:21
@sffc
Copy link
Contributor Author

sffc commented Jan 30, 2023

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.

Copy link
Contributor

@gibson042 gibson042 left a 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.

spec/pluralrules.html Outdated Show resolved Hide resolved
spec/pluralrules.html Outdated Show resolved Hide resolved
spec/pluralrules.html Show resolved Hide resolved
spec/pluralrules.html Show resolved Hide resolved
Copy link
Member

@ryzokuken ryzokuken left a 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.

spec/negotiation.html Show resolved Hide resolved
</emu-clause>

<emu-clause id="sec-formatnumericrangetoparts" aoid="FormatNumericRangeToParts">
<h1>FormatNumericRangeToParts ( _numberFormat_, _x_, _y_ )</h1>
Copy link
Member

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.

Copy link
Contributor Author

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. :)

<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>
Copy link
Member

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...

Copy link
Contributor

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>.

Copy link
Contributor Author

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~, &laquo; *"ceil"*, *"floor"*, *"expand"*, *"trunc"*, *"halfCeil"*, *"halfFloor"*, *"halfExpand"*, *"halfTrunc"*, *"halfEven"* &raquo;, *"halfExpand"*).
1. Let _trailingZeroDisplay_ be ? GetOption(_options_, *"trailingZeroDisplay"*, ~string~, &laquo; *"auto"*, *"stripIfInteger"* &raquo;, *"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.
Copy link
Member

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

Copy link
Contributor Author

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 Show resolved Hide resolved
spec/numberformat.html Show resolved Hide resolved
<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_.
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

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_.
Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a sentence

spec/numberformat.html Show resolved Hide resolved
spec/numberformat.html Show resolved Hide resolved
Copy link
Contributor

@gibson042 gibson042 left a 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 Show resolved Hide resolved
spec/numberformat.html Outdated Show resolved Hide resolved
1. Else,
1. Set _intlObj_.[[RoundingType]] to ~fractionDigits~.
1. Set _intlObj_.[[RoundingType]] to ~morePrecision~.
Copy link
Contributor

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~.

Copy link
Contributor Author

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 Show resolved Hide resolved
Comment on lines +409 to +414
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"*).
Copy link
Contributor

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())?

Copy link
Contributor Author

@sffc sffc Apr 5, 2023

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.

Copy link
Contributor

@gibson042 gibson042 Apr 5, 2023

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 Show resolved Hide resolved
spec/numberformat.html Outdated Show resolved Hide resolved
</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>
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Arrow emoji SGTM

@sffc sffc requested a review from gibson042 April 5, 2023 02:12
spec/numberformat.html Outdated Show resolved Hide resolved
spec/numberformat.html Outdated Show resolved Hide resolved
spec/numberformat.html Outdated Show resolved Hide resolved
spec/numberformat.html Outdated Show resolved Hide resolved
Comment on lines +409 to +414
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"*).
Copy link
Contributor

@gibson042 gibson042 Apr 5, 2023

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.

Copy link
Contributor

@gibson042 gibson042 left a 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>
@sffc
Copy link
Contributor Author

sffc commented Apr 5, 2023

Thanks for the suggestions; they all looked good to me

@sffc sffc requested a review from gibson042 April 5, 2023 20:33
@ryzokuken
Copy link
Member

@ryzokuken Thoughts on the above? I can live with it, albeit unhappily.

@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.

@gibson042
Copy link
Contributor

gibson042 commented Apr 6, 2023

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: …, roundingMode, roundingIncrement, trailingZeroDisplay, roundingPriority.

The property ordering is already awkward, which is basically why I can live this, but my preference is still to avoid making things worse.

@gibson042 gibson042 merged commit 4257160 into master Apr 6, 2023
@gibson042 gibson042 deleted the proposal-nfv3 branch April 6, 2023 17:39
ben-allen pushed a commit to ben-allen/ecma402 that referenced this pull request Apr 10, 2023
ben-allen pushed a commit to ben-allen/ecma402 that referenced this pull request Apr 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants