-
Notifications
You must be signed in to change notification settings - Fork 153
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
Relax error checking requiring exclusive end in NudgeToCalendarUnit #2924
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2924 +/- ##
==========================================
- Coverage 96.51% 96.26% -0.26%
==========================================
Files 23 23
Lines 12432 12073 -359
Branches 2258 2206 -52
==========================================
- Hits 11999 11622 -377
- Misses 374 387 +13
- Partials 59 64 +5 ☔ View full report in Codecov by Sentry. |
Updated per #2919 (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.
89d170a
to
d8f28b0
Compare
I wrote a test case that passes with this branch and fails on main: tc39/test262#4196 |
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 can't test if the proposed changes will always compute the expected results until I've updated SpiderMonkey's Temporal implementation to use the current DifferenceISODate
. This won't happen before finishing the changes for #2925, so it'll probably take some time.
d8f28b0
to
e3eb821
Compare
I'm going to roll this one into #2925 because test262 tests have already been merged for it. That also allows dropping the "Editorial: Downgrade assertion back to exception" commit, because without custom calendars that invariant can no longer be broken. |
Merged as part of #2925. |
Fix for #2919
Looks like the sanity checking was a little too aggressive. Was asserting a range with an exclusive end. Should be inclusive end instead. To see what I mean, I'll breaking down the operations from #2919 ...