-
Notifications
You must be signed in to change notification settings - Fork 170
Handle non-ISO-calendar edge case with leap years and differencing #3176
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
Conversation
|
test262 PR: tc39/test262#4636 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3176 +/- ##
==========================================
+ Coverage 96.66% 96.67% +0.01%
==========================================
Files 22 22
Lines 10321 10357 +36
Branches 1857 1859 +2
==========================================
+ Hits 9977 10013 +36
Misses 294 294
Partials 50 50 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ptomato
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this fixes some cases but regresses others.
d1 = new Temporal.PlainDate(2016, 3, 31, 'chinese');
d2 = new Temporal.PlainDate(2019, 3, 29, 'chinese');
d1.until(d2, { largestUnit: 'years' })
// old: P3Y
// new: P37M
// The old result is dubious (looks like it might need to be P3Y1M) but the new result seems wrong in any case
d1 = new Temporal.PlainDate(2019, 5, 1, 'chinese');
d2 = new Temporal.PlainDate(2020, 4, 19, 'chinese');
d1.until(d2, { largestUnit: 'years' })
// old: P1Y
// new: P12M
// The old result seems correct (Chinese year 2019 is a common year)I suspect this is because the assumption mentioned in the comment is incorrect: that the difference is less than a year when we hit the problematic case.
I've pushed the in-progress snapshots I've been working with to https://github.com/tc39/proposal-temporal/compare/chinese-hebrew-snapshots?expand=1 so you can do the same testing I'm doing.
11668dd to
1c455a1
Compare
ptomato
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks correct to me. I have just a couple of nitpicks.
polyfill/lib/calendar.mjs
Outdated
| } | ||
| const diffYears = calendarTwo.year - calendarOne.year; | ||
| // Take the difference between the years of the two dates | ||
| let diffYears = calendarTwo.year - calendarOne.year; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
ptomato
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
See #3159