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 non-ISO calendar month arithmetic #1761

Merged
merged 3 commits into from
Aug 25, 2021

Conversation

justingrant
Copy link
Collaborator

@justingrant justingrant commented Aug 23, 2021

Addresses three bugs with non-ISO date math. Fixes #1760. Fixes #1768.

This PR includes the following changes to the non-production polyfill:

  • Fixes add/subtract when the starting day of the month was longer than the shortest month between the start and endpoint, e.g. adding a month to Jan 31 or subtracting a month from Mar 31 in the Gregorian calendar.
  • Fixes until/since where largestUnit was 'months' or 'years' and the second argument was more than one month later than the first argument
  • Fixes until/since where largestUnit was 'months' or 'years' where weeks were being incorrectly returned.
  • Increases test coverage to cover these three fixes, plus all of datemath.mjs demitasse tests.

Because non-ISO calendar implementations are part of ECMA 402, not ECMA 262, this won't result in any spec changes nor docs changes. Only the non-prod polyfill is affected, but it's a polite to fix bug anyways so we won't have obviously broken results for some calendars.

@codecov
Copy link

codecov bot commented Aug 23, 2021

Codecov Report

Merging #1761 (f6fc705) into main (e3f66bb) will increase coverage by 0.08%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1761      +/-   ##
==========================================
+ Coverage   94.81%   94.89%   +0.08%     
==========================================
  Files          19       19              
  Lines       10764    10782      +18     
  Branches     1725     1729       +4     
==========================================
+ Hits        10206    10232      +26     
+ Misses        545      537       -8     
  Partials       13       13              
Flag Coverage Δ
test262 73.30% <0.00%> (-0.11%) ⬇️
tests 90.14% <100.00%> (+0.13%) ⬆️

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

Impacted Files Coverage Δ
polyfill/lib/calendar.mjs 94.30% <100.00%> (+0.45%) ⬆️

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 e3f66bb...f6fc705. Read the comment docs.

@justingrant justingrant marked this pull request as draft August 23, 2021 21:19
@justingrant
Copy link
Collaborator Author

Converted to draft, because I think there may be a separate bug happening with subtraction of months. If there is, I'll fix it too before un-drafting this.

@justingrant
Copy link
Collaborator Author

Converted to draft, because I think there may be a separate bug happening with subtraction of months. If there is, I'll fix it too before un-drafting this.

Fixed the subtraction bug too and added tests for it. Removing draft status.

@justingrant justingrant marked this pull request as ready for review August 24, 2021 01:17
@justingrant justingrant changed the title Fix non-ISO calendar month addition Fix non-ISO calendar month addition and subtraction Aug 24, 2021
justingrant added a commit to justingrant/temporal-polyfill that referenced this pull request Aug 24, 2021
Copy link
Collaborator

@ptomato ptomato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't looked closely at why this works but I think we should probably test gregory at least as well as we do iso8601, so it might be worth applying this patch https://gist.github.com/ptomato/adcedda30013fc750c7f4f57bd1427d1 to run the (hopefully exhaustive) datemath tests again with the gregory calendar. (The complication is that doing this reveals that there's a bug somewhere in non-ISO dateUntil as well, even without this PR)

polyfill/lib/calendar.mjs Outdated Show resolved Hide resolved
@justingrant
Copy link
Collaborator Author

I think we should probably test gregory at least as well as we do iso8601, so it might be worth applying this patch https://gist.github.com/ptomato/adcedda30013fc750c7f4f57bd1427d1 to run the (hopefully exhaustive) datemath tests again with the gregory calendar. (The complication is that doing this reveals that there's a bug somewhere in non-ISO dateUntil as well, even without this PR)

Great idea! I'd prefer to do it in a separate PR though. I could fix dateUntil there, as well as any other bugs that show up. Want to file an issue to track, ideally with a failing dateUntil repro?

@ptomato
Copy link
Collaborator

ptomato commented Aug 24, 2021

I think we should probably test gregory at least as well as we do iso8601, so it might be worth applying this patch https://gist.github.com/ptomato/adcedda30013fc750c7f4f57bd1427d1 to run the (hopefully exhaustive) datemath tests again with the gregory calendar. (The complication is that doing this reveals that there's a bug somewhere in non-ISO dateUntil as well, even without this PR)

Great idea! I'd prefer to do it in a separate PR though. I could fix dateUntil there, as well as any other bugs that show up. Want to file an issue to track, ideally with a failing dateUntil repro?

I've done this in #1768.

Adding and subtracting months was broken for non-ISO months when the
starting point was near the end of the calendar month.

This commit fixes this issue and adds tests for this case.
Fixes tc39#1768 which was caused by assuming that `two` was always later
than `one` when `largestUnit` was `'years'` or `'months'`.

Also included `calendar: 'gregory'` in datemath demitasse tests.
While fixing another bug, I noticed that `weeks` was being returned
by non-ISO dateUntil when largestUnit is `months` or `years`.

There were existing demitasse test cases that should have covered this
codepath, but they weren't adding enough days to trigger the bug. Now
they do.
@justingrant justingrant changed the title Fix non-ISO calendar month addition and subtraction Fix non-ISO calendar month arithmetic Aug 25, 2021
@justingrant
Copy link
Collaborator Author

I think we should probably test gregory at least as well as we do iso8601, so it might be worth applying this patch https://gist.github.com/ptomato/adcedda30013fc750c7f4f57bd1427d1 to run the (hopefully exhaustive) datemath tests again with the gregory calendar. (The complication is that doing this reveals that there's a bug somewhere in non-ISO dateUntil as well, even without this PR)

Great idea! I'd prefer to do it in a separate PR though. I could fix dateUntil there, as well as any other bugs that show up. Want to file an issue to track, ideally with a failing dateUntil repro?

I've done this in #1768.

Adapting these tests and fixing that bug turned out to be really easy. So I just included the changes here. Also fixed another bug I found in the neighborhood.

Copy link
Collaborator

@ptomato ptomato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

polyfill/test/intl.mjs Show resolved Hide resolved
@ptomato ptomato merged commit 96e67db into tc39:main Aug 25, 2021
justingrant added a commit to justingrant/temporal-polyfill that referenced this pull request Aug 27, 2021
ptomato pushed a commit to js-temporal/temporal-polyfill that referenced this pull request Aug 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants