Conversation
Replace TODO placeholders in the Amount constructor and convertTo with actual CreateFormatterObject calls that build the formatter object needed by FormatNumericToString. Closes #93
|
gibson042
left a comment
There was a problem hiding this comment.
This is a change for the better, but is not currently addressing #89 (comment) .
Co-authored-by: Richard Gibson <richard.gibson@gmail.com>
There was a problem hiding this comment.
This looks good after fixing the typing to correctly map integral Numbers to integers where required.
And for a potential followup, there's a common pattern here that has me wondering whether a new operation is warranted:
1. If _x_ is not *undefined*, set _x_ to ? SnapToInteger(_x_, ~strict~, 1, 21).
where the second parameter is either ~strict~ to reject non-integral output from ToNumber or ~truncate-strict~ to reject non-finite output but truncate finite non-integers, and the optional third and fourth parameters respectively define inclusive lower and upper bounds.
Such an operation could replace Temporal ToIntegerIfIntegral(x) with SnapToInteger(x, ~strict~), Temporal ToIntegerWithTruncation(x) with SnapToInteger(x, ~truncate-strict~), and Temporal ToPositiveIntegerWithTruncation(x) with SnapToInteger(x, ~truncate-strict~, 1). It could potentially even be extended to consolidate almost all integer conversions by introducing a ~truncate-loose~ behavior (truncating finite non-integer output from ToNumber and mapping NaN to 0) for use in the definition of ToLength and ToIndex (respectively, 𝔽(the result of clamping SnapToInteger(argument, ~truncate-loose~) between 0 and 253 - 1) and SnapToInteger(argument, ~truncate-loose~, 0, 253 - 1)):
| Operation | NaN | ±∞ | Finite non-integer | Bounds | Behavior |
|---|---|---|---|---|---|
| ToIntegerOrInfinity | → 0 | preserve | truncate | n/a | |
| ToIntegerIfIntegral (Temporal) | RangeError | RangeError | RangeError | STRICT | |
| ToIntegerWithTruncation (Temporal) | RangeError | RangeError | truncate | TRUNCATE-STRICT | |
| ToPositiveIntegerWithTruncation (Temporal) | RangeError | RangeError | truncate | RangeError if ≤ 0 | TRUNCATE-STRICT |
| ToLength | → 0 | preserve | truncate | clamp to [0, 2**53 - 1] | TRUNCATE-LOOSE |
| ToIndex | → 0 | preserve | truncate | RangeError if outside [0, 2**53 - 1] | TRUNCATE-LOOSE |
| DefaultNumberOption (ECMA-402) | RangeError | RangeError | preserve | RangeError if outside [min, max]1 | n/a1 |
| GetAmountOptions | RangeError | RangeError | RangeError | RangeError if outside [min, max] | STRICT |
| GetAmountConvertToOptions | RangeError | RangeError | RangeError | RangeError if outside [min, max] | STRICT |
Footnotes
-
DefaultNumberOption currently checks output from ToNumber against specified bounds before truncation: https://github.com/tc39/ecma402/issues/691 ↩ ↩2
Co-authored-by: Richard Gibson <richard.gibson@gmail.com>
Co-authored-by: Richard Gibson <richard.gibson@gmail.com>
Co-authored-by: Richard Gibson <richard.gibson@gmail.com>
gibson042
left a comment
There was a problem hiding this comment.
LGTM, and note the below improvement suggestions.
Co-authored-by: Richard Gibson <richard.gibson@gmail.com>
Co-authored-by: Richard Gibson <richard.gibson@gmail.com>
Co-authored-by: Richard Gibson <richard.gibson@gmail.com>
Co-authored-by: Richard Gibson <richard.gibson@gmail.com>
Adds the CreateFormatterObject abstract operation, which builds the formatter object (with internal slots for rounding mode, digit limits, etc.) needed by FormatNumericToString. Replaces the TODO placeholders in the Amount constructor and
convertTowith actual calls to this new AO. Spun off from #89 per reviewer feedback. Closes #93.