Normative: Fix ZonedDateTime::round incorrect results with smallestUnit:day #2797
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2797 +/- ##
==========================================
+ Coverage 96.51% 96.60% +0.09%
==========================================
Files 23 23
Lines 12386 12304 -82
Branches 2294 2276 -18
==========================================
- Hits 11954 11886 -68
+ Misses 372 356 -16
- Partials 60 62 +2 ☔ View full report in Codecov by Sentry. |
ptomato
left a comment
ptomato
left a comment
There was a problem hiding this comment.
Thanks for doing this, and the test262 tests as well! I'm pretty sure I understand it and it seems correct to me. Good to know that there were actually a few wrong tests in test262, not just adjustments to the observable user calls.
To present this to TC39 we'll need a change to the spec text. Are you interested in taking on that task as well?
|
@ptomato, sorry for the delay. I was on vaca for 1 week and upon returning have lots to catch up on. Will try my best to address all feedback by EOW. |
No problem. I think we are still in plenty of time for the TC39 agenda review deadline, 2024-03-29. If you would like help with the spec text, I'd be happy to contribute that part if it will let you concentrate on catching up with other things. |
|
Hi @ptomato, I addressed all feed back and also added some tests that will make all new code have coverage. Writing the spec text was driving me nuts as I'm unfamiliar with the notation and conventions. I'd greatly appreciate it if you could do it. I'm happy to do any further refinements to make this compatible with what TC39 expects. |
|
@arshaw I've got a draft of the spec text, would you mind checking the "Allow edits from maintainers" checkbox? Then I can push it to this branch in a new commit. Could you also submit the relevant test262 tests in fullcalendar/test262 as a PR to tc39/test262? I would offer to do it and save you some time, but then the UI won't let me approve my own PR. |
|
@ptomato, the checkbox didn't show up I believe because you are not a collaborator on fullcalendar/proposal-temporal. I invited you. Hope fully the checkbox shows up after accepting. I've created the test262 PR here: |
|
That's odd - it specifically shouldn't be necessary for me to be a collaborator on the source repo and that's never happened to me before! But once I accepted the invitation, I was able to add the commit. Sorry about that! |
|
@ptomato, I built and reviewed your spec changes, and everything looks as expected. Thank you so much. |
ptomato
changed the title
|
This change reached consensus at the TC39 meeting of 2024-04-08. |
The following code shows how ZonedDateTime would previously get rounded up
by two days instead of the intended 1 day:
const zdt0 = Temporal.ZonedDateTime.from('2024-03-10T23:00:01[America/New_York]')
const zdt1 = zdt0.round({ smallestUnit: 'day', roundingMode: 'ceil' })
console.log(zdt1.toString()) // '2024-03-12T00:00:00-04:00[America/New_York]'
- 23:00:01 is considered the numerator
- 23:00:00 is considered the denominator (because 2024-03-10 has 23 hours
in this tz)
- Dividing results in a number slightly larger than 1
- The ceil func nudges it to 2 (i.e. 2 days from 2024-03-10T00:00:00)
This is the spec text corresponding to the reference code change in the
previous commit.
Closes: tc39#2790
ptomato
force-pushed
the
2790-round-zdt-day
branch
from
f464da6 to
31fcba2
Compare
|
Tests failed due to the formatting snapshot of |
|
test262 fix is in tc39/test262#4048. If it gets merged within a few days, I will update this PR to pull it in. If not, I'll add that test to the expected failures file and merge this. |
ptomato
force-pushed
the
2790-round-zdt-day
branch
from
31fcba2 to
4e9ef15
Compare
3 participants
This is my proposed fix to #2790
Adjusted tests:
tc39/test262@main...fullcalendar:test262:temporal-2790-round-zdt-day
Could someone please review?