Normative: Simplify Duration parsing #1907
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1907 +/- ##
==========================================
+ Coverage 89.85% 94.92% +5.07%
==========================================
Files 17 19 +2
Lines 10929 10991 +62
Branches 1582 1721 +139
==========================================
+ Hits 9820 10433 +613
+ Misses 1071 543 -528
+ Partials 38 15 -23
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
gibson042
changed the title
There was a problem hiding this comment.
This looks great! I had not made the connection with the ParseText machinery. I assume we should use ParseText in all the other AOs that parse our ISO 8601 grammar.
I have a hunch that we could adopt as much of this change as would be possible in a separate editorial-only PR, and keep the actual normative bug-fixing PR small? I think that would help delegates review the normative change for December. Is it possible to split this up with that in mind?
gibson042
force-pushed
the
2021-11-simplify-parse-duration
branch
from
c39358b to
b3ec4c4
Compare
If adoption of ParseText is not considered normative then I think this PR is there now, with the first two commits accomplishing that and the remainder removing DurationHandleFractions and fixing #1754 in the process by ignoring duration sign until the end of the operation. |
|
I would guess that the adoption of ParseText is not normative, because I don't think it would be possible to write a test that would distinguish whether an implementation was following the spec text before or after that change. So, the only one that needs to be presented in December is "Normative: Remove DurationHandleFractions"? Can we change the order of the commits so that "Editorial: Ignore sign until the end" and "Editorial: Assign MVs to new aliases rather than replacing Parse Nodes" are applied to the current spec text, and then preserved when moving code around in "Normative: Remove DurationHandleFractions"? (I'm happy to do this if you think it's an OK idea, I find rebasing patches relaxing) |
Yes, feel free! But I'm not sure you'll be able to meaningfully reorder "Ignore sign until the end", because calling DurationHandleFractions with only nonnegative values will probably implement the normative change that fixes #1754. |
ptomato
force-pushed
the
2021-11-simplify-parse-duration
branch
from
b3ec4c4 to
7a21347
Compare
|
OK, I've rebased this. |
ptomato
added
the
needs plenary input
|
This achieved consensus at the December 2021 TC39 meeting. |
tc39/proposal-temporal#1907 was a bug that caused negative Duration strings with fractional units to be rounded incorrectly. Add tests that ensure the rounding mode is correct. This was a normative change that achieved consensus at the December 2021 TC39 meeting.
tc39/proposal-temporal#1907 was a bug that caused negative Duration strings with fractional units to be rounded incorrectly. Add tests that ensure the rounding mode is correct. This was a normative change that achieved consensus at the December 2021 TC39 meeting.
tc39/proposal-temporal#1907 was a bug that caused negative Duration strings with fractional units to be rounded incorrectly. Add tests that ensure the rounding mode is correct. This was a normative change that achieved consensus at the December 2021 TC39 meeting.
...as mentioned in #1759. This also fixes some spec bugs from use of
floorwith negative values.This also requires care with respect to using mathematical values rather than approximations, and I'd like to see a test for input like ±PT46H66M71.50040904S (which produced an off-by-one error at the nanoseconds level when I was evaluating a JS-based implementation of the new algorithm that took too many liberties in that respect).
Fixes #1754