polyfill: durations accept fractional time values #1179

ryzokuken · 2020-11-12T20:34:52Z

Fixes: #938

codecov · 2020-11-12T20:36:09Z

Codecov Report

Merging #1179 (b5f1c99) into main (5930868) will increase coverage by 2.46%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1179      +/-   ##
==========================================
+ Coverage   93.55%   96.01%   +2.46%     
==========================================
  Files          19       19              
  Lines        7961     9265    +1304     
  Branches     1264     1404     +140     
==========================================
+ Hits         7448     8896    +1448     
+ Misses        506      363     -143     
+ Partials        7        6       -1

Flag	Coverage Δ
test262	`38.24% <ø> (-3.37%)`	⬇️
tests	`92.15% <ø> (+2.71%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
polyfill/lib/intrinsicclass.mjs
polyfill/lib/shim.mjs
polyfill/lib/legacydate.mjs
polyfill/lib/slots.mjs
lib/intrinsicclass.mjs	`59.09% <0.00%> (ø)`
lib/ecmascript.mjs	`96.31% <0.00%> (ø)`
lib/temporal.mjs	`100.00% <0.00%> (ø)`
lib/plainmonthday.mjs	`91.32% <0.00%> (ø)`
lib/legacydate.mjs	`0.00% <0.00%> (ø)`
lib/zoneddatetime.mjs	`97.80% <0.00%> (ø)`
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4589e83...b4a56f1. Read the comment docs.

justingrant

Looks good, with one question and a few suggestions re: test cases.

polyfill/lib/ecmascript.mjs

polyfill/lib/regex.mjs

justingrant · 2020-11-12T23:20:29Z

polyfill/test/duration.mjs

-        'P1Y1M1W1DT0.5H',
-        'P1Y1M1W1DT1H0,5M',
-        'P1Y1M1W1DT1H0.5M0.5S'
+        { years: 0.5 },


IMHO, all these rounding tests should be done with both positive and negative durations, because a frequent cause of rounding errors in libraries is differences in handling negative vs. positive numbers.

ptomato

Looking good, the main change is that I think we should throw on more than our supported maximum number of decimal places, as per ISO 8601.

polyfill/lib/ecmascript.mjs

ptomato · 2020-11-13T01:54:47Z

polyfill/lib/ecmascript.mjs

+      fHours,
+      minutes,
+      fMinutes,
+      seconds,


I think it shouldn't be necessary to pass the seconds, milliseconds, microseconds and nanoseconds into DurationHandleFractions when parsing a string: the result is already handled exactly in the existing code. You can do something like this:

({ minutes, seconds } = ES.DurationHandleFractions(fHours, minutes, fMinutes, 0, 0, 0, 0, 0, 0, 0, 0);

and let the seconds and sub-second units be determined as they already were.

(Although then you do need to ensure that the seconds are not given in the string, if minutes are fractional.)

(Although then you do need to ensure that the seconds are not given in the string, if minutes are fractional.)

Exactly why I did it this way.

polyfill/lib/ecmascript.mjs

polyfill/lib/regex.mjs

ptomato · 2020-11-13T02:00:48Z

polyfill/lib/ecmascript.mjs

@@ -793,15 +918,15 @@ export const ES = ObjectAssign({}, ES2020, {
    if (validUnits.indexOf(unit1) > validUnits.indexOf(unit2)) return unit2;
    return unit1;
  },
-  ToPartialRecord: (bag, fields) => {
+  ToPartialRecord: (bag, fields, cast = ES.ToInteger) => {


I'm not sure the spec language has first-class abstract operations like this, but, maybe consider instead splitting BUILTIN_FIELDS into BUILTIN_DATETIME_FIELDS and DURATION_FIELDS, and using ToNumber on duration fields?

Fixes: tc39#938

ptomato · 2020-11-16T00:48:04Z

I noticed from the CI output that the "order of operations" test262 tests supply non-integer input, those are breaking now instead of truncating it.

littledan · 2020-11-16T19:34:21Z

As I explained in #938 (comment) , I think this PR should be dropped.

ptomato · 2020-11-16T20:08:53Z

This PR still implements support for ISO strings such as PT0.5H, which is unrelated to the Number concern, I think?

ptomato

I don't think we should block this PR on any of the remaining comments, since they are minor improvements, so I think punting them to a followup is fine.

spec/abstractops.html

ptomato · 2020-11-17T03:03:48Z

spec/abstractops.html

@@ -949,10 +987,10 @@ <h1>ISO 8601 grammar</h1>
          Digits TimeFraction? SecondsDesignator

      DurationMinutes :
-          Digits MinutesDesignator DurationSeconds?
+          Digits TimeFraction? MinutesDesignator DurationSeconds?


I think the grammar should show that the fraction is incompatible with the seconds part:

DurationMinutes : Digits MinutesDesignator DurationSeconds? Digits TimeFraction MinutesDesignator

What we're doing right now is accept a duration with all the fractions, set the units to zero appropriately and handle them that way. We could change the spec text and instead of throwing at a later time, removing all those checks etc.

I am not 100% sure, but I think PT0.5H30M is actually illegal according to ISO 8601. In the polyfill it doesn't matter whether we throw because it's not accepted by the regex or we throw right after, but I think the spec grammar should match what's a legal string as much as possible.

Correct. Only the smallest unit is permitted to be fractional in ISO 8601.

The string still throws, it just throws after the parsing stage (in DurationHandleFractions). I thought it was okay as long as we threw on the wrong format.

ptomato · 2020-11-17T03:04:36Z

spec/abstractops.html

@@ -1198,42 +1236,51 @@ <h1>ParseTemporalDurationString ( _isoString_ )</h1>
      1. Assert: Type(_isoString_) is String.
      1. If _isoString_ does not satisfy the syntax of a |TemporalDurationString| (see <emu-xref href="#sec-temporal-iso8601grammar"></emu-xref>), then
        1. Throw a *RangeError* exception.
-      1. Let _sign_, _years_, _months_, _weeks_, _days_, _hours_, _minutes_, _seconds_, and _fraction_ be the parts of _isoString_ produced respectively by the |Sign|, |DurationYears|, |DurationMonths|, |DurationWeeks|, |DurationDays|, |DurationHours|, |DurationMinutes|, |DurationSeconds|, and |TimeFractionalPart| productions, or *undefined* if not present.
+      1. Let _sign_, _years_, _months_, _weeks_, _days_, _hours_, _fHours_, _minutes_, _fMinutes_, _seconds_, and _fSeconds_ be the parts of _isoString_ produced respectively by the |Sign|, |DurationYears|, |DurationMonths|, |DurationWeeks|, |DurationDays|, |DurationHours|, |DurationHoursFraction|, |DurationMinutes|, |DurationMinutesFraction|, |DurationSeconds|, and |DurationSecondsFraction| productions, or *undefined* if not present.


DurationHoursFraction, DurationMinutesFraction, and DurationSecondsFraction need to be defined.

can we use |TimeFractionalPart| three times over?

Yes, I think you can just define them like DurationHoursFraction : TimeFractionalPart etc.

- Fix order-of-operations tests (which used 1.7 as a value for durations and relied upon it being truncated to 1, which we probably don't want regardless of the state of this) - Remove Duration.from subclass-invalid-arg test, which is no longer valid; the subclass constructor is never called with a non-float argument - Fix ecmarkup linter errors

ptomato · 2020-11-17T03:39:29Z

OK, merging this now, we should solve the remaining comments in a follow up soon, ideally when the discussion in #938 is resolved.

ryzokuken added this to the Stable proposal milestone Nov 12, 2020

ryzokuken self-assigned this Nov 12, 2020

justingrant requested changes Nov 12, 2020

View reviewed changes

ptomato approved these changes Nov 13, 2020

View reviewed changes

littledan mentioned this pull request Nov 13, 2020

Should Duration.from accept non-integer values? (presumably for time units only) #938

Closed

ryzokuken added 3 commits November 16, 2020 02:22

polyfill: durations accept fractional time values

9e7fa0f

Fixes: tc39#938

fixup! polyfill: durations accept fractional time values

31cbd3e

fixup! polyfill: durations accept fractional time values

148f2b0

ryzokuken force-pushed the duration/non-integer branch from 9c79f9e to 148f2b0 Compare November 15, 2020 20:52

spec: add spec for handling fractional durations

5103821

ptomato approved these changes Nov 17, 2020

View reviewed changes

ptomato merged commit fc86767 into tc39:main Nov 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

polyfill: durations accept fractional time values #1179

polyfill: durations accept fractional time values #1179

ryzokuken commented Nov 12, 2020

codecov bot commented Nov 12, 2020 •

edited

Loading

justingrant left a comment

justingrant Nov 12, 2020

ptomato left a comment

ptomato Nov 13, 2020

ryzokuken Nov 15, 2020

ptomato Nov 13, 2020

ptomato commented Nov 16, 2020

littledan commented Nov 16, 2020

ptomato commented Nov 16, 2020

ptomato left a comment

ptomato Nov 17, 2020

ryzokuken Nov 17, 2020

ptomato Nov 17, 2020

justingrant Nov 17, 2020

ryzokuken Nov 17, 2020

ptomato Nov 17, 2020

ryzokuken Nov 17, 2020

ptomato Nov 17, 2020

ptomato commented Nov 17, 2020

polyfill: durations accept fractional time values #1179

polyfill: durations accept fractional time values #1179

Conversation

ryzokuken commented Nov 12, 2020

codecov bot commented Nov 12, 2020 • edited Loading

Codecov Report

justingrant left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ptomato left a 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

ptomato commented Nov 16, 2020

littledan commented Nov 16, 2020

ptomato commented Nov 16, 2020

ptomato left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ptomato commented Nov 17, 2020

codecov bot commented Nov 12, 2020 •

edited

Loading