Negative durations #811

ptomato · 2020-08-11T00:52:05Z

This is a huge pull request; I'd suggest reviewing it commit by commit.

The first two commits are small unrelated fixes. The third is a small rearrangement. The fourth actually allows negative durations, but the difference() methods still throw if called on two objects in the wrong order. The fifth commit fixes that and allows all the difference() methods to return negative durations.

Closes: #558
Closes: #782
Closes: #645

codecov · 2020-08-11T00:53:27Z

Codecov Report

Merging #811 into main will increase coverage by 0.15%.
The diff coverage is 99.47%.

@@            Coverage Diff             @@
##             main     #811      +/-   ##
==========================================
+ Coverage   92.74%   92.89%   +0.15%     
==========================================
  Files          17       17              
  Lines        4881     5055     +174     
  Branches      754      773      +19     
==========================================
+ Hits         4527     4696     +169     
- Misses        347      352       +5     
  Partials        7        7

Flag	Coverage Δ
#test262	`56.48% <52.57%> (-1.03%)`	⬇️
#tests	`88.01% <98.16%> (+0.40%)`	⬆️

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

Impacted Files	Coverage Δ
polyfill/lib/calendar.mjs	`67.82% <89.47%> (+1.15%)`	⬆️
polyfill/lib/absolute.mjs	`96.39% <100.00%> (ø)`
polyfill/lib/date.mjs	`90.87% <100.00%> (-0.57%)`	⬇️
polyfill/lib/datetime.mjs	`94.04% <100.00%> (+0.10%)`	⬆️
polyfill/lib/duration.mjs	`97.70% <100.00%> (+0.68%)`	⬆️
polyfill/lib/ecmascript.mjs	`94.74% <100.00%> (-0.21%)`	⬇️
polyfill/lib/regex.mjs	`100.00% <100.00%> (ø)`
polyfill/lib/time.mjs	`97.17% <100.00%> (+0.55%)`	⬆️
polyfill/lib/yearmonth.mjs	`94.95% <100.00%> (-0.30%)`	⬇️
... and 1 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 102b0be...93c64e2. Read the comment docs.

docs/balancing.md

docs/duration.md

polyfill/lib/ecmascript.mjs

docs/duration.md

polyfill/lib/regex.mjs

polyfill/test/duration.mjs

docs/time.md

justingrant · 2020-08-11T05:42:28Z

Nice! I left a few comments, mostly on the docs. Nothing major. I didn't dig deeply into the polyfill implementation or the specs.

It's nice to see how much simpler a few of the cookbook samples got with negative durations.

Thanks for doing this-- huge improvement!

docs/balancing.md

ptomato · 2020-08-12T17:49:56Z

Thanks for the reviews! There were definitely some good catches in there.

ptomato · 2020-08-15T01:45:47Z

This now requires #826.

ryzokuken

Partially reviewed, and wow, it's huge.

spec/time.html

docs/duration.md

polyfill/lib/date.mjs

polyfill/lib/time.mjs

spec/duration.html

ptomato · 2020-08-24T22:51:44Z

Thanks for the review, definitely caught some stuff I didn't see! I believe it's ready for review again.

sffc

It looks good from spot-checking, but I didn't do a thorough review.

ryzokuken

What a long pull request! Final few suggestions. Thanks a ton, @ptomato!

docs/duration.md

polyfill/lib/datetime.mjs

spec/time.html

ryzokuken · 2020-09-01T11:28:47Z

spec/duration.html

+    <emu-clause id="sec-temporal-durationsign" aoid="DurationSign">
+      <h1>DurationSign ( _years_, _months_, _weeks_, _days_, _hours_, _minutes_, _seconds_, _milliseconds_, _microseconds_, _nanoseconds_ )</h1>
+      <emu-alg>
+        1. For each value _v_ of _years_, _months_, _weeks_, _days_, _hours_, _minutes_, _seconds_, _milliseconds_, _microseconds_, _nanoseconds_, do


Suggested change

1. For each value _v_ of _years_, _months_, _weeks_, _days_, _hours_, _minutes_, _seconds_, _milliseconds_, _microseconds_, _nanoseconds_, do

1. For each value _v_ of « _years_, _months_, _weeks_, _days_, _hours_, _minutes_, _seconds_, _milliseconds_, _microseconds_, _nanoseconds_ », do

I'm not sure what the correct spec language is, but there seems to be a precedent for not using the angle brackets: https://tc39.es/ecma402/#sec-todatetimeoptions

ryzokuken · 2020-09-01T11:29:20Z

spec/duration.html

+      <h1>RejectDurationSign ( _years_, _months_, _weeks_, _days_, _hours_, _minutes_, _seconds_, _milliseconds_, _microseconds_, _nanoseconds_ )</h1>
+      <emu-alg>
+        1. Let _sign_ be ! DurationSign(_years_, _months_, _weeks_, _days_, _hours_, _minutes_, _seconds_, _milliseconds_, _microseconds_, _nanoseconds_).
+        1. For each value _v_ of _years_, _months_, _weeks_, _days_, _hours_, _minutes_, _seconds_, _milliseconds_, _microseconds_, _nanoseconds_, do


ryzokuken · 2020-09-01T11:30:12Z

spec/duration.html

    <emu-clause id="sec-temporal-validatetemporalduration" aoid="ValidateTemporalDuration">
      <h1>ValidateTemporalDuration ( _years_, _months_, _weeks_, _days_, _hours_, _minutes_, _seconds_, _milliseconds_, _microseconds_, _nanoseconds_ )</h1>
      <emu-alg>
-        1. For each of _years_, _months_, _weeks_, _days_, _hours_, _minutes_, _seconds_, _milliseconds_, _microseconds_, _nanoseconds_, if any of the values is negative or infinite, return *false*.
+        1. Let _sign_ be ! DurationSign(_years_, _months_, _weeks_, _days_, _hours_, _minutes_, _seconds_, _milliseconds_, _microseconds_, _nanoseconds_).
+        1. For each value _v_ of _years_, _months_, _weeks_, _days_, _hours_, _minutes_, _seconds_, _milliseconds_, _microseconds_, _nanoseconds_, do


ryzokuken · 2020-09-01T11:30:54Z

spec/duration.html

        1. If _disambiguation_ is *"reject"*, then
          1. If ! ValidateTemporalDuration(_years_, _months_, _weeks_, _days_, _hours_, _minutes_, _seconds_, _milliseconds_, _microseconds_, _nanoseconds_) is *false*, then
            1. Throw a *RangeError* exception.
        1. Else if _disambiguation_ is *"constrain"*, then
-          1. For each of _years_, _months_, _weeks_, _days_, _hours_, _minutes_, _seconds_, _milliseconds_, _microseconds_, _nanoseconds_, if any of the values is negative, throw a *RangeError* exception.
-          1. For each of _years_, _months_, _weeks_, _days_, _hours_, _minutes_, _seconds_, _milliseconds_, _microseconds_, _nanoseconds_, if any of the values is infinite, let it be the largest possible finite value of the Number type.
+          1. For each of _years_, _months_, _weeks_, _days_, _hours_, _minutes_, _seconds_, _milliseconds_, _microseconds_, _nanoseconds_, if any of the values is infinite, let it be the sign of that value times the largest possible finite value of the Number type.


Does the last comment apply here too? Not sure, but maybe.

It's odd that we have AddTime but not SubtractTime. Regardless, we'll need to have it as a separate operation since it will be used from two places for negative duration support.

Changes Temporal.Duration to allow negative values in the fields, as long as all the fields have the same sign. Adds negated() and abs() methods to Temporal.Duration, and a sign property. On all types with arithmetic, subtracting a negative duration is equivalent to adding the absolute value of that duration, and adding a negative duration is equivalent to subtracting the absolute value of that duration. Closes: #782

This undoes the work in commit d84653e although it's not exactly a revert, because the situation before that was that durations were always positive. We also don't reinstate the behaviour of never returning a duration larger than 12 hours. Now, for all types, calling smaller.difference(larger) will result in a negative duration. See: #782

See: #814

ptomato requested review from Ms2ger, justingrant, pipobscure, jasonwilliams, sffc, gibson042 and ryzokuken August 11, 2020 00:52