Normative: Reorder NF resolved option "roundingPriority" #768

gibson042 · 2023-04-06T18:01:57Z

The second commit is a normative reordering of property sequence; the first commit is a supporting editorial change that should land in some form even if the normative change is rejected.

…to initialization

ljharb · 2023-04-06T18:09:06Z

spec/numberformat.html

-        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"*).


this seems normative (on the editorial commit) - does adding it to the table below cause it to be implicitly set on options instead?

Yes.

There is a normative change, which is that the order in which the append order for the options is changing slightly: previously roundingPriority was at the end, but now it is second from the end.

In the case, the commit is mislabeled; it'd help my review, at least, to split up the editorial and normative parts.

The editorial and normative parts are separated, as explained in the PR description.

1ee3bee39e4b2bc21cb9105b7fba2c0c7cef4627 is editorial, moving the infallible computation of rounding priority from NumberFormat.prototype.resolvedOptions to the Intl.NumberFormat constructor and updating the resolvedOptions algorithm to add a "roundingPriority" property from the table in step 5 rather than from special steps following table row iteration (but still adding "roundingPriority" as the observably last property).

0d936078aaf3d83b114ae092f4ac80e7bbfbcf85 is normative, reordering table rows such that "roundingPriority" will be grouped with the other "rounding…" properties and observably precede "trailingZeroDisplay".

This comment is on the editorial commit.

So what part of «Editorial: Move computation of NF resolved option "roundingPriority" to initialization» is mislabeled?

@gibson042 that was my question. I only looked at the editorial commit, and I saw the line I'm commenting on here. @sffc said it's normative. Your comment implies that it's not normative ("but still"), which was what i'd been asking about.

It appears to me that the editorial commit is editorial, and the normative commit is normative

sffc · 2023-04-07T01:28:52Z

TG2 discussion (was on the problem, before seeing the concrete PR): https://github.com/tc39/ecma402/blob/master/meetings/notes-2023-04-06.md#nfv3-resolvedoptions

sffc

LGTM; let's seek approval from the rest of the group.

sffc · 2023-04-08T00:31:40Z

spec/numberformat.html

-        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"*).


Yes.

There is a normative change, which is that the order in which the append order for the options is changing slightly: previously roundingPriority was at the end, but now it is second from the end.

sffc · 2023-05-04T21:57:52Z

TG2 discussion: https://github.com/tc39/ecma402/blob/master/meetings/notes-2023-05-04.md#normative-reorder-nf-resolved-option-roundingpriority

sffc · 2023-06-30T12:05:57Z

TG2 discussion again: https://github.com/tc39/ecma402/blob/master/meetings/notes-2023-06-29.md#normative-reorder-nf-resolved-option-roundingpriority

Still needs tests; otherwise has TG2 approval.

FrankYFTang · 2023-07-12T00:08:29Z

could someone explain to me why do we bother to make this PR to change the order of "roundingPriority" in the resolvedOptions() to be placed before "trailingZeroDisplay" but in the mean time KEEP the order of "roundingIncrement" after "roundingMode" and not change it to be placed before "roundingMode"???

gibson042 · 2023-07-12T00:49:11Z

@sffc provided a partial answer at tc39/proposal-intl-numberformat-v3#122 (comment) . But the whole thing is clearly a mess anyway, and at this point I'd be equally happy to rearrange "roundingIncrement" as well (or to keep "roudingPriority" at the end and reduce this PR to the editorial commit that just fixes the Intl.NumberFormat.prototype.resolvedOptions algorithm).

sffc · 2023-07-12T09:31:46Z

This PR puts the three rounding modifiers (roundingMode, roundingIncrement, and roundingPriority) next to each other. Why is there desire to change the order amongst those three? At least they are adjacent now.

FrankYFTang · 2023-07-12T23:43:08Z

Here is the READING order of these options now as July 12, 2023)

7. Let roundingPriority be ? GetOption(options, "roundingPriority", string, « "auto", "morePrecision", "lessPrecision" », "auto").
8. Let roundingIncrement be ? GetNumberOption(options, "roundingIncrement", 1, 5000, 1).
9. If roundingIncrement is not in « 1, 2, 5, 10, 20, 25, 50, 100, 200, 250, 500, 1000, 2000, 2500, 5000 », throw a RangeError exception.
10. Let roundingMode be ? GetOption(options, "roundingMode", string, « "ceil", "floor", "expand", "trunc", "halfCeil", "halfFloor", "halfExpand", "halfTrunc", "halfEven" », "halfExpand").
11. Let trailingZeroDisplay be ? GetOption(options, "trailingZeroDisplay", string, « "auto", "stripIfInteger" », "auto").

It seems there are no dependency between these four option readings in the current shape of ECMA402, right?

After this PR merged, the order in the resolvedOptions between these four will be

[[RoundingMode]] | "roundingMode"
[[RoundingIncrement]] | "roundingIncrement"
[[ComputedRoundingPriority]] | "roundingPriority"
[[TrailingZeroDisplay]] | "trailingZeroDisplay"

so... the new order in both the option reading AND the resolvedOptions()
a. Are NOT alphabetical
b. Are NOT in the same order

so... if the order is really matter and require a PR to change it, would it be better to
A1. Change the option reading order to alphatical between these four?
A2. Change the resolvedOptions() order also to be the same as the reading order?

or at least
B. Change the resolvedOptions() order also to be the same as the reading order if we decide not to do A1?
or
C. Change the resolvedOptions() order between these two to be alphatical if we decide not to do A1+A2 or B?

I am not going to block you from merging this PR at this point since it passed TC39 consensus this week already. But I just have hard time to understand the need to change from what we had to this "half baked" state.

FrankYFTang · 2023-07-13T00:44:10Z

v8 bug tracking https://bugs.chromium.org/p/v8/issues/detail?id=14169
v8 cl https://chromium-review.googlesource.com/c/v8/v8/+/4679292

@gibson042
If you decide to merge this PR as is , then please do so and I will land v8 cl 4679292 after you merge it
But if you feel we should reexam the order of both the option reading and the resolvedOptions based on what I just mentioned above then I will hold the cl 4679292 .

ryzokuken · 2023-07-21T19:38:13Z

Let's take this to a follow-on issue. I'll merge this.

FrankYFTang · 2023-07-24T18:31:04Z

Could you merge this ASAP? My landing of https://chromium-review.googlesource.com/c/v8/v8/+/4679292 is blocked by the merging of this PR.

ryzokuken · 2023-07-26T18:29:52Z

@FrankYFTang sorry I missed this, done.

Ref #768 * Read "roundingIncrement", "roundingMode", "roundingPriority", "trailingZeroDisplay" in alphabetic order. * Output "pluralCategories", "roundingIncrement", "roundingMode", "roundingPriority", "trailingZeroDisplay" in alphabetic order.

Ref tc39#768 * Read "roundingIncrement", "roundingMode", "roundingPriority", "trailingZeroDisplay" in alphabetic order. * Output "pluralCategories", "roundingIncrement", "roundingMode", "roundingPriority", "trailingZeroDisplay" in alphabetic order.

gibson042 added 2 commits April 6, 2023 13:56

Editorial: Move computation of NF resolved option "roundingPriority" …

1ee3bee

…to initialization

Normative: Reorder NF resolved option "roundingPriority"

0d93607

gibson042 requested review from sffc and ryzokuken April 6, 2023 18:01

ljharb reviewed Apr 6, 2023

View reviewed changes

sffc approved these changes Apr 8, 2023

View reviewed changes

ryzokuken approved these changes Apr 11, 2023

View reviewed changes

ryzokuken added s: discuss Status: TG2 must discuss to move forward normative labels May 4, 2023

ryzokuken added has consensus Has consensus from TC39-TG2 and removed s: discuss Status: TG2 must discuss to move forward labels Jun 29, 2023

ryzokuken merged commit 0fbf16c into tc39:master Jul 26, 2023

FrankYFTang mentioned this pull request Aug 3, 2023

Normative: Fix order of rounding* option reads and resolvedOptions() #811

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Normative: Reorder NF resolved option "roundingPriority" #768

Normative: Reorder NF resolved option "roundingPriority" #768

gibson042 commented Apr 6, 2023

ljharb Apr 6, 2023

sffc Apr 8, 2023

ljharb Apr 8, 2023

gibson042 Apr 8, 2023

ljharb Apr 8, 2023

gibson042 Apr 9, 2023

ljharb Apr 9, 2023

sffc Jul 12, 2023

sffc commented Apr 7, 2023

sffc left a comment

sffc Apr 8, 2023

sffc commented May 4, 2023

sffc commented Jun 30, 2023

FrankYFTang commented Jul 12, 2023

gibson042 commented Jul 12, 2023

sffc commented Jul 12, 2023

FrankYFTang commented Jul 12, 2023 •

edited

Loading

FrankYFTang commented Jul 13, 2023

ryzokuken commented Jul 21, 2023

FrankYFTang commented Jul 24, 2023

ryzokuken commented Jul 26, 2023

Normative: Reorder NF resolved option "roundingPriority" #768

Normative: Reorder NF resolved option "roundingPriority" #768

Conversation

gibson042 commented Apr 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sffc commented Apr 7, 2023

sffc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sffc commented May 4, 2023

sffc commented Jun 30, 2023

FrankYFTang commented Jul 12, 2023

gibson042 commented Jul 12, 2023

sffc commented Jul 12, 2023

FrankYFTang commented Jul 12, 2023 • edited Loading

FrankYFTang commented Jul 13, 2023

ryzokuken commented Jul 21, 2023

FrankYFTang commented Jul 24, 2023

ryzokuken commented Jul 26, 2023

FrankYFTang commented Jul 12, 2023 •

edited

Loading