-
Notifications
You must be signed in to change notification settings - Fork 105
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
Editorial: Refactor SetNumberFormatDigitOptions #575
Conversation
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.
Great change, it makes everything so much simpler editorially! I wish it was here before #471! One minor comment about simplifying one of the branches.
spec/numberformat.html
Outdated
1. Let _mxsd_ be ? DefaultNumberOption(_mxsd_, _mnsd_, 21, 21). | ||
1. Set _intlObj_.[[MinimumSignificantDigits]] to _mnsd_. | ||
1. Set _intlObj_.[[MaximumSignificantDigits]] to _mxsd_. | ||
1. Else, |
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.
Since _needSd_
is the same as _hasSd_
, why do we need this?
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.
Maybe it is there just to align this with the next condition? Does that achieve much though?
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.
The difference becomes apparent in NFv3.
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.
@sffc ah, okay, I suspected so... perhaps it is okay to drop these additional conditions here and re-add them in NFv3?
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 have a slight preference to keep the extra variables because (1) they more clearly signify intent and (2) they reduce the size of the NFv3 diff, but I'm willing to change it if you feel more strongly.
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.
No, I don't feel that strongly.
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 resolved this a bit by denesting the hasSd construction.
ECMASpeak doesn't have implicit to boolean conversions in this context. So this needs to be more like:
|
@sffc could you please address the review? |
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.
How's this?
spec/numberformat.html
Outdated
1. Let _mxsd_ be ? DefaultNumberOption(_mxsd_, _mnsd_, 21, 21). | ||
1. Set _intlObj_.[[MinimumSignificantDigits]] to _mnsd_. | ||
1. Set _intlObj_.[[MaximumSignificantDigits]] to _mxsd_. | ||
1. Else, |
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 resolved this a bit by denesting the hasSd construction.
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.
Just two editorial questions, everything else LGTM.
spec/numberformat.html
Outdated
1. If _mnsd_ is not *undefined* or _mxsd_ is not *undefined*, set _hasSd_ to *true*; else, set _hasSd_ to *false*. | ||
1. If _mnfd_ is not *undefined* or _mxfd_ is not *undefined*, set _hasFd_ to *true*; else, set _hasFd_ to *false*. |
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.
Any reason why both of these conditions are on a single line instead of being split across two?
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.
Because I find it more readable to write
- If condition, set a to true; else, set a to false.
- If condition, set b to true; else, set b to false.
- If condition, set c to true; else, set c to false.
- If condition, set d to true; else, set d to false.
than to write
- If condition, set a to true.
- Else, set a to false.
- If condition, set b to true.
- Else, set b to false.
- If condition, set c to true.
- Else, set c to false.
- If condition, set d to true.
- Else, set d to false.
But if you disagree, or if my syntax is not normal for the spec, I will change it.
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 that it's more readable, but I am unsure if (1) this is a spec convention (2) it even matters.
Maybe we can ask the 262 editors about it?
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.
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.
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.
Both alternatives are valid and are used in ECMA-262 and ECMA-402.
I think as a rule of thumb, I normally just try to follow the usage in the surrounding code resp. operations. Except when the condition is overly long or in a deeply nested step, so that the short form would either be rendered across multiple lines or when readability suffers.
spec/numberformat.html
Outdated
1. If _mnsd_ is not *undefined* or _mxsd_ is not *undefined*, set _hasSd_ to *true*; else, set _hasSd_ to *false*. | ||
1. If _mnfd_ is not *undefined* or _mxfd_ is not *undefined*, set _hasFd_ to *true*; else, set _hasFd_ to *false*. | ||
1. Let _needSd_ be _hasSd_. | ||
1. If not _hasSd_ and _notation_ is not *"compact"*, set _needFd_ to *true*; else, set _needFd_ to *false*. |
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.
Same as above.
Since you're making changes to SetNumberFormatDigitOptions, can you also improve its description and eliminate the initial assertion steps by adopting a structured header (cf. #594)?
|
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.
This change appears to be normative: https://jsfiddle.net/x0jq9drc/
Example observable impact:
SetNumberFormatDigitOptions(intlObj, {maximumFractionDigits: 0}, 0, 3, "compact")
[[MinimumIntegerDigits]]: 1
-[[RoundingType]]: ~fractionDigits~
-[[MinimumFractionDigits]]: 0
-[[MaximumFractionDigits]]: 0
+[[RoundingType]]: ~compactRounding~
1. Set _hasFd_ to *true*. | ||
1. Else, | ||
1. Set _hasFd_ to *false*. | ||
1. Let _needSd_ be _hasSd_. |
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.
What is the motivation for realiasing hasSd rather than continuing to use it directly?
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 like the parallelism between [has/need]Fd with [has/need]Sd.
Also, in NumberFormat v3, needSd no longer equals hasSd. I could merge the variables for now, but doing so will increase the diff relative to NFv3.
Co-authored-by: Richard Gibson <[email protected]>
Co-authored-by: Richard Gibson <[email protected]>
Done!
Your JSFiddle is outstanding! I made one diff in dd7d0cf and managed to make your test case pass. Thank you so much! (Is it possible to get something like this into Test262? @ryzokuken) |
spec/numberformat.html
Outdated
1. If _needSd_ or _needFd_, then | ||
1. If _hasSd_, then |
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.
1. If _needSd_ or _needFd_, then | |
1. If _hasSd_, then | |
1. If _needSd_ is *true* or _needFd_ is *true*, then | |
1. If _hasSd_ is *true*, then |
But for that matter, this part seems hard to follow. Broader suggestion:
1. If _needSd_ is *false* and _needFd_ is *false*, then
1. Set _intlObj_.[[RoundingType]] to ~compactRounding~.
1. Else if _hasSd_ is *true*, then
1. Set _intlObj_.[[RoundingType]] to ~significantDigits~.
1. Else,
1. Set _intlObj_.[[RoundingType]] to ~fractionDigits~.
Co-authored-by: Richard Gibson <[email protected]>
Ping |
@gibson042 could you please give this another review? It would be great to merge this in ASAP in order to move things along for NF v3. |
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.
This looks good to me, and I confirmed that it does not include normative changes: https://jsfiddle.net/8teq2va9/
But as an editorial nit, I would like to see a followup that makes the parameter and alias names more clear. e.g.:
- mnfdDefault → minFractionalDigitsDefault
- mxfdDefault → maxFractionalDigitsDefault
- mnid → minIntegerDigits
- hasSd → constrainsSignificantDigits
- …
Co-authored-by: Richard Gibson <[email protected]>
Looks like this is resolved! Thanks @gibson042 and @sffc. Merging this. |
fwiw, /cc @ljharb any idea what's wrong? |
This is an editorial change intended to re-structure the SetNumberFormatDigitOptions abstract operation to reduce diffs in the NumberFormat v3 proposal.
@ryzokuken, you've worked extensively on this abstract operation before, so I would like you to review it to make sure that it is, indeed, editorial.