ZonedDateTime hoursInDay, startOfDay, round, add, subtract #1103

ptomato · 2020-11-03T02:23:06Z

Co-authored-by: Justin Grant <justingrant@users.noreply.github.com> See: #569

Note this implementation is incorrect for rounding to the nearest day on days where a DST transition takes place. This case will be handled correctly when we implement ZonedDateTime difference algorithms for since() and until(). There are two tests dealing with these cases which are skipped for the time being. Co-authored-by: Justin Grant <justingrant@users.noreply.github.com> See: #569

This operation will be reused in ZonedDateTime.add() and subtract() as well. See: #569

Two tests are skipped due to the change in order-of-operations that will land in #993. Co-authored-by: Justin Grant <justingrant@users.noreply.github.com> See: #569

The message is no longer accurate, since ZonedDateTime itself has landed already. Change it to say which methods are as yet unimplemented.

codecov · 2020-11-03T02:24:13Z

Codecov Report

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

@@            Coverage Diff             @@
##             main    #1103      +/-   ##
==========================================
- Coverage   93.55%   90.16%   -3.39%     
==========================================
  Files          19       17       -2     
  Lines        7961     8178     +217     
  Branches     1264     1182      -82     
==========================================
- Hits         7448     7374      -74     
- Misses        506      794     +288     
- Partials        7       10       +3

Flag	Coverage Δ
test262	`?`
tests	`90.16% <ø> (+0.73%)`	⬆️

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

Impacted Files	Coverage Δ
polyfill/lib/legacydate.mjs
polyfill/lib/regex.mjs
polyfill/lib/time.mjs
polyfill/lib/yearmonth.mjs
polyfill/lib/now.mjs
polyfill/lib/date.mjs
polyfill/lib/duration.mjs
polyfill/lib/datetime.mjs
polyfill/lib/temporal.mjs
polyfill/lib/intl.mjs
... and 23 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 e8dbc65...e6594b3. Read the comment docs.

justingrant

Looks great. It's so cool to see ZDT coming alive! I made a few minor suggestions and notes. Nothing major.

polyfill/lib/zoneddatetime.mjs

justingrant · 2020-11-03T04:23:26Z

polyfill/lib/zoneddatetime.mjs

+    const timeZone = GetSlot(this, TIME_ZONE);
+    const todayNs = GetSlot(ES.GetTemporalInstantFor(timeZone, today, 'compatible'), EPOCHNANOSECONDS);
+    const tomorrowNs = GetSlot(ES.GetTemporalInstantFor(timeZone, tomorrow, 'compatible'), EPOCHNANOSECONDS);
+    let { seconds, milliseconds, microseconds, nanoseconds } = ES.DifferenceInstant(


Why is all the code below here needed? Could it be replaced with something like this?

return Number(tomorrowNs - todayNs) / 3.6e12;

Or is there some non-ISO time calendar reason to want to do it the way you did?

Good point. We still do need to calculate the quotient and remainder separately because BigInt division is integer division, but this can be simplified a lot.

Why not just convert the difference to a Number and then use floating-point Number math? If a "day" is longer than 140 24-hour days, some precision loss seems OK. ;-)

In the spec text, the epoch nanoseconds are mathematical values (ℝ values) so you have to be explicit about when you convert them to Number values, my guess is that this is the cleanest way.

polyfill/test/zoneddatetime.mjs

justingrant · 2020-11-03T04:30:54Z

polyfill/lib/zoneddatetime.mjs

+    const timeZone = GetSlot(this, TIME_ZONE);
+    const offsetNs = ES.GetOffsetNanosecondsFor(timeZone, GetSlot(this, INSTANT));
+
+    // See the 'prefer' algorithm in ToTemporalZonedDateTime


I don't fully follow this logic here, but as long as tests pass I assume it's fine.

polyfill/test/zoneddatetime.mjs

justingrant · 2020-11-03T04:41:41Z

polyfill/lib/ecmascript.mjs

+
+    // RFC 5545 requires the date portion to be added in calendar days and the
+    // time portion to be added in exact time.
+    // FIXME: "op" and the dateAdd/dateSubtract conditional will not be needed


This comment doesn't match the code below. What conditional?

Sorry, left over from a previous version where I was trying to keep both AddZonedDateTime and SubtractZonedDateTime consolidated into one function. I'll remove this.

Cool. Please leave the note about #993 though! Will be helpful for me to find places to fix along with #993. I'm assuming that these AOs should be consolidated the #993 fix-- LMK if not.

BTW, I'm hoping to finish the #993 fix today. Tomorrow at latest.

There is still a note about #993 in SubtractZonedDateTime, so that should be fine.

polyfill/test/zoneddatetime.mjs

Ms2ger

Thanks for the careful review, @justingrant. It seems like none of your/my comments particularly justify holding this up, so I'll merge now, and let @ptomato deal with them in the next PR.

Ms2ger · 2020-11-03T09:31:03Z

spec/zoneddatetime.html

+        1. Let _isoCalendar_ be ? GetISO8601Calendar().
+        1. Let _today_ be ? CreateTemporalDateTime(_year_, _month_, _day_, 0, 0, 0, 0, 0, 0, _isoCalendar_).
+        1. Let _tomorrowFields_ be ? AddDate(_year_, _month_, _day_, 0, 0, 0, 1, *"reject"*).
+        1. Let _tomorrow_ be ? CreateTemporalDateTime(_tomorrowFields_.[[ISOYear]], _tomorrowFields_.[[ISOMonth]], _tomorrowFields_.[[ISODay]], 0, 0, 0, 0, 0, 0, _isoCalendar_).


Check if tomorrowFields's fields start with ISO

Ms2ger · 2020-11-03T09:59:22Z

spec/zoneddatetime.html

+        1. Let _rounded_ be ? CreateTemporalDateTime(_roundResult_.[[Year]], _roundResult_.[[Month]], _roundResult_.[[Day]], _roundResult_.[[Hour]], _roundResult_.[[Minute]], _roundResult_.[[Second]], _roundResult_.[[Millisecond]], _roundResult_.[[Microsecond]], _roundResult_.[[Nanosecond]], _calendar_).
+        1. Let _offsetNanoseconds_ be ? GetOffsetNanosecondsFor(_timeZone_, _instant_).
+        1. Let _possibleInstants_ be ? GetPossibleInstantsFor(_timeZone_, _rounded_).
+        1. If there exists an element _candidate_ of _possibleInstants_ for which ? GetOffsetNanosecondsFor(_timeZone_, _candidate_) = _offsetNanoseconds_, then


Should make the order of operations explicit, since all the GetOffsetNanosecondsFor calls are observable

ptomato · 2020-11-03T19:16:05Z

Thanks for the reviews, I've got fixes for all of these on my next branch.

ptomato added 7 commits November 2, 2020 16:00

Fix typo in spec

5ad9cf8

Implement ZonedDateTime.startOfDay()

6060f43

Co-authored-by: Justin Grant <justingrant@users.noreply.github.com> See: #569

Implement ZonedDateTime.hoursInDay

749de65

Co-authored-by: Justin Grant <justingrant@users.noreply.github.com> See: #569

Refactor Instant.add() and subtract() into abstract operation

74e8453

This operation will be reused in ZonedDateTime.add() and subtract() as well. See: #569

Implement ZonedDateTime.add() and ZonedDateTime.subtract()

9f70b96

Two tests are skipped due to the change in order-of-operations that will land in #993. Co-authored-by: Justin Grant <justingrant@users.noreply.github.com> See: #569

Update messages about landing ZonedDateTime

e6594b3

The message is no longer accurate, since ZonedDateTime itself has landed already. Change it to say which methods are as yet unimplemented.

ptomato requested review from Ms2ger, justingrant, pipobscure, jasonwilliams, sffc, gibson042 and ryzokuken November 3, 2020 02:23

justingrant approved these changes Nov 3, 2020

View reviewed changes

Ms2ger approved these changes Nov 3, 2020

View reviewed changes

Ms2ger merged commit a368ad3 into main Nov 3, 2020

Ms2ger deleted the zoneddatetime-3 branch November 3, 2020 10:06

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ZonedDateTime hoursInDay, startOfDay, round, add, subtract #1103

ZonedDateTime hoursInDay, startOfDay, round, add, subtract #1103

ptomato commented Nov 3, 2020

codecov bot commented Nov 3, 2020

justingrant left a comment

justingrant Nov 3, 2020

justingrant Nov 3, 2020

ptomato Nov 3, 2020

justingrant Nov 3, 2020

ptomato Nov 3, 2020

justingrant Nov 3, 2020

justingrant Nov 3, 2020

ptomato Nov 3, 2020

justingrant Nov 3, 2020

justingrant Nov 3, 2020

ptomato Nov 3, 2020

Ms2ger left a comment

Ms2ger Nov 3, 2020

Ms2ger Nov 3, 2020

ptomato commented Nov 3, 2020

ZonedDateTime hoursInDay, startOfDay, round, add, subtract #1103

ZonedDateTime hoursInDay, startOfDay, round, add, subtract #1103

Conversation

ptomato commented Nov 3, 2020

codecov bot commented Nov 3, 2020

Codecov Report

justingrant left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ms2ger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ptomato commented Nov 3, 2020