Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix order of operations in subtract and direction in since #1123

Merged
merged 11 commits into from
Nov 5, 2020

Conversation

justingrant
Copy link
Collaborator

@justingrant justingrant commented Nov 5, 2020

MERGE THIS PR BEFORE #1116. @ptomato will update that PR so its tests will work with the changes in this PR.

#1121 may also break this PR... not sure though. Ideally this PR could be merged first.

This PR fixes #993 by:

  • Revising subtract() methods to use largest-units-first order of operations
  • Fixing since() methods to use largest-units-first order of operations and to always use this as the starting point for both arithmetic and rounding.
  • A few test changes to adjust to new results and to test the new OOO
  • Removing dateSubtract & timeSubtract from Calendar.
  • Removing various *Subtract abstract operations

I split this up into multiple commits to make it easier to review. Some of the commits are trivial changes (e.g. deleting stuff from specs and docs, or prettier changes) while others have logic that needs review.

If you're in a hurry, the most important commits to review are these:

  • 39b6f60 - changes to the DifferenceDate abstract operation
  • bba2278 - changes to subtract and since in various Temporal types

Follow-ups needed:

  • Additional OOO tests - e.g. tests for since and tests for DateTime and YearMonth.
  • Make spec updates from #1123 changes #1124 Spec updates. Other than trivial changes to the YearMonth spec, this PR doesn't include spec updates to account for changes to ES.DifferenceDate, *.subtract, or *.since

This was the first time prettier was run on this file, so many format
and whitespace changes. No content changes.
Other specs aren't updated yet, but this change was trivial.
Changes for `since` are still TBD here and in other specs.
This commit contains changes to remove Calendar's dateSubtract and
timeSubtract methods, as well as the DateSubtract abstract operation.

No changes in this checkin actually change any logic, so it should be
quicker to review than the other commits in this PR.
This commit updates the ISO calendar's `dateAdd` to work with negative durations,
and removes dateSubtract & timeSubtract methods from the implementation.
Updates `ES.DifferenceDate` to perform reversible, largest-units-first
date arithmetic.  I didn't spend a lot of time optimizing, so the
algorithm can surely be improved later.
Now that order-of-operations works, these tests now pass.
This commit changes Date, DateTIme, YearMonth, and Time to:
* Revise `subtract` to use largest-units-first order of operations
* Fixes `since` to use largest-units-first order of operations and to
  always use `this` as the starting point for both arithmetic and
  rounding.
* A few test changes to adjust to new results and to test the new OOO
@codecov
Copy link

codecov bot commented Nov 5, 2020

Codecov Report

Merging #1123 into main will decrease coverage by 3.38%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1123      +/-   ##
==========================================
- Coverage   93.55%   90.17%   -3.39%     
==========================================
  Files          19       17       -2     
  Lines        7961     8261     +300     
  Branches     1264     1202      -62     
==========================================
+ Hits         7448     7449       +1     
- Misses        506      801     +295     
- Partials        7       11       +4     
Flag Coverage Δ
test262 ?
tests 90.17% <ø> (+0.73%) ⬆️

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

Impacted Files Coverage Δ
polyfill/lib/regex.mjs
polyfill/lib/slots.mjs
polyfill/lib/legacydate.mjs
polyfill/lib/intrinsicclass.mjs
polyfill/lib/intl.mjs
polyfill/lib/shim.mjs
polyfill/lib/temporal.mjs
lib/instant.mjs 86.64% <0.00%> (ø)
lib/datetime.mjs 90.84% <0.00%> (ø)
lib/slots.mjs 100.00% <0.00%> (ø)
... and 14 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 2961d38...eadd02b. Read the comment docs.

polyfill/test/date.mjs Outdated Show resolved Hide resolved
@justingrant
Copy link
Collaborator Author

Test262 is still broken in CI, but I just pushed a 2-character commit that fixes Test262 when run locally. Now all tests pass.

@Ms2ger Ms2ger merged commit d8d8947 into tc39:main Nov 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

difference should always return a "top-heavy balanced" duration with largest-first order of operations
2 participants