Editorial: Consolidate GetOption usage #1531
Conversation
|
This is the promised follow up from #1505. The spec text is identical to what was reviewed there. What has changed is a bug fix to make the polyfill spec-compliant, and the addition of all the test262 tests. |
Codecov Report
@@ Coverage Diff @@
## main #1531 +/- ##
=======================================
Coverage 95.64% 95.64%
=======================================
Files 19 19
Lines 10923 10927 +4
Branches 1715 1717 +2
=======================================
+ Hits 10447 10451 +4
Misses 465 465
Partials 11 11
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| 1. Return ? DefaultNumberOption(_value_, _minimum_, _maximum_, _fallback_). | ||
| </emu-alg> | ||
| </emu-clause> | ||
|
|
||
| <emu-clause id="sec-getstringornumberoption" aoid="GetStringOrNumberOption"> | ||
| <h1>GetStringOrNumberOption ( _options_, _property_, _stringValues_, _minimum_, _maximum_, _fallback_ )</h1> |
There was a problem hiding this comment.
there doesn't appear to be an assertion (in prose, or in the steps) that minimum and maximum is a Number, but without that assertion, the comparison to value would be incoherent.
There was a problem hiding this comment.
In #1413 it seems to be suggested that comparisons should be done on mathematical values, so I'll sort this out there.
| @@ -266,7 +240,9 @@ <h1>ToTemporalRoundingIncrement ( _normalizedOptions_, _dividend_, _inclusive_ ) | |||
| 1. Let _maximum_ be _dividend_ − 1. | |||
| 1. Else, | |||
| 1. Let _maximum_ be 1. | |||
| 1. Let _increment_ be ? GetNumberOption(_normalizedOptions_, *"roundingIncrement"*, 1, _maximum_, 1). | |||
| 1. Let _increment_ be ? GetOption(_normalizedOptions_, *"roundingIncrement"*, « Number », *undefined*, 1). | |||
| 1. If _increment_ < 1 or _increment_ > _maximum_, throw a *RangeError* exception. | |||
There was a problem hiding this comment.
this also lacks an assertion that dividend is a Number (thus ensuring maximum is a Number), but also, 1 is a mathemetical value.
In other words, a series of assertions/conversions is required here so the number types are the same.
There was a problem hiding this comment.
I'd rather do this as part of #1413, to keep the scope of this change from expanding more than it already has...
spec/abstractops.html
Outdated
| @@ -266,7 +240,9 @@ <h1>ToTemporalRoundingIncrement ( _normalizedOptions_, _dividend_, _inclusive_ ) | |||
| 1. Let _maximum_ be _dividend_ − 1. | |||
| 1. Else, | |||
| 1. Let _maximum_ be 1. | |||
| 1. Let _increment_ be ? GetNumberOption(_normalizedOptions_, *"roundingIncrement"*, 1, _maximum_, 1). | |||
| 1. Let _increment_ be ? GetOption(_normalizedOptions_, *"roundingIncrement"*, « Number », *undefined*, 1). | |||
There was a problem hiding this comment.
| 1. Let _increment_ be ? GetOption(_normalizedOptions_, *"roundingIncrement"*, « Number », *undefined*, 1). | |
| 1. Let _increment_ be ℝ(? GetOption(_normalizedOptions_, *"roundingIncrement"*, « Number », *undefined*, 1)). |
perhaps? this way the comparisons below can be to mathematical values instead of to Numbers
There was a problem hiding this comment.
Likewise, I'll sort this out in #1413.
Under the assumption that it's not a priority to move GetNumberOption directly from ECMA-402 into ECMA-262, reorganize the GetOption operations: - Change the _type_ string argument to GetOption, into a List of valid types, the last one of which will be coerced to if the value is none of them (and also not undefined). - In the Number case, call ToNumber on the value and throw if it is NaN. - Inline GetNumberOption as it's only called once. - Call GetOption in GetStringOrNumberOption. - Don't delete GetOption, GetNumberOption, and DefaultNumberOption from 402, as they won't be replaced by anything in 262 at present. Includes a fix to make the polyfill spec-compliant, and test262 tests covering usages of GetOption (there are quite a lot of them.) Closes: #1411
Under the assumption that it's not a priority to move GetNumberOption
directly from ECMA-402 into ECMA-262, reorganize the GetOption operations:
Change the type string argument to GetOption, into a List of valid
types, the last one of which will be coerced to if the value is none of
them (and also not undefined).
In the Number case, call ToNumber on the value and throw if it is NaN.
Inline GetNumberOption as it's only called once.
Call GetOption in GetStringOrNumberOption.
Don't delete GetOption, GetNumberOption, and DefaultNumberOption from
402, as they won't be replaced by anything in 262 at present.
Includes a fix to make the polyfill spec-compliant, and test262 tests
covering usages of GetOption (there are quite a lot of them.)
Closes: #1411