Introduce calendar systems to time #1062

ryzokuken · 2020-10-27T14:50:14Z

Fixes: #522

ptomato

This is looking good, is the plan to merge it and work on PRs for documentation and spec separately, or add those to this PR?

I do think that tests for time.getISOFields() should be added as part of this PR though.

As well, we could consider adding a calendar to test/usercalendar.mjs that exercises this functionality.

polyfill/lib/calendar.mjs

polyfill/lib/date.mjs

polyfill/lib/datetime.mjs

polyfill/lib/time.mjs

ptomato

Sorry, I missed the first time around, that Time.add, Time.subtract, Time.until, and Time.since need to use calendar.timeAdd, calendar.timeSubtract, calendar.timeUntil.

polyfill/lib/calendar.mjs

polyfill/lib/datetime.mjs

justingrant · 2020-10-27T22:59:23Z

polyfill/lib/calendar.mjs

+    void constructor;
+    throw new Error('not implemented');
+  }
+  timeSubtract(time, duration, options, constructor) {


Why do we need both timeAdd and timeSubtract? Why not just timeAdd? Same question for dates, FWIW. Also, given that both time and date both use calendars now, why have separate methods for them? Why not just have a dateTimeAdd that serves both purposes? I assume there are good reasons for the split, just wondering what they are.

Related: #1066 (for dates). But if we're adding time methods for the first time, why not just have one?

The original calendar draft had only one add() and one subtract() method. I can't remember exactly, but it may not be possible to combine dateAdd() and timeAdd() since the allowed ranges for all the Plain types are different. But it may be possible to keep that range checking in the callers.

justingrant · 2020-10-27T23:03:08Z

polyfill/lib/date.mjs

-  NANOSECOND,
+  ISO_HOUR,
+  ISO_MINUTE,
+  ISO_SECOND,


For seconds and smaller units, do they need to be calendar-aware? Are there any use cases we know about that require calendar-specific measurement of seconds or nanoseconds? Or are we just making them calendar-aware for consistency?

I think we should probably rename them to ISO in any case, but yes it does seem that there should not be use cases where 1000 milliseconds ≠ 1 second.

justingrant · 2020-10-28T05:29:44Z

I didn't review this deeply because I'm not familiar enough with the implementation, but I left a few notes.

One other related thing I noticed: the phrase "calendar units" shows up in a bunch of places in Temporal polyfill code and a few places in the spec. In the past these were synonyms for "may not be fixed-length units" but now that all units are calendar units, we probably need a different term to replace "calendar units" with. How about "fixed-length units" and "variable-length units" ?

polyfill/lib/datetime.mjs

codecov · 2020-10-29T11:05:27Z

Codecov Report

Merging #1062 into main will decrease coverage by 52.19%.
The diff coverage is 47.20%.

@@             Coverage Diff             @@
##             main    #1062       +/-   ##
===========================================
- Coverage   93.59%   41.40%   -52.20%     
===========================================
  Files          19       18        -1     
  Lines        7702     3794     -3908     
  Branches     1224      775      -449     
===========================================
- Hits         7209     1571     -5638     
- Misses        486     2003     +1517     
- Partials        7      220      +213

Flag	Coverage Δ
test262	`41.40% <47.20%> (+0.29%)`	⬆️
tests	`?`

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

Impacted Files	Coverage Δ
polyfill/lib/date.mjs	`33.74% <0.00%> (-59.88%)`	⬇️
polyfill/lib/intl.mjs	`12.41% <0.00%> (-87.59%)`	⬇️
polyfill/lib/timezone.mjs	`46.78% <ø> (-47.97%)`	⬇️
polyfill/lib/zoneddatetime.mjs	`0.41% <0.00%> (-68.72%)`	⬇️
polyfill/lib/time.mjs	`31.66% <40.65%> (-65.96%)`	⬇️
polyfill/lib/calendar.mjs	`42.37% <59.72%> (-41.24%)`	⬇️
polyfill/lib/datetime.mjs	`45.86% <63.41%> (-49.83%)`	⬇️
polyfill/lib/ecmascript.mjs	`46.04% <63.88%> (-50.51%)`	⬇️
polyfill/lib/slots.mjs	`100.00% <100.00%> (ø)`
... and 16 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 366351b...8e4de7b. Read the comment docs.

ptomato

I'm afraid that Duration rounding and balancing will be a big missing part of this, but let's make sure that what is in this PR can be merged soon.

ptomato · 2020-10-30T22:39:46Z

polyfill/lib/datetime.mjs

-      nanosecond,
-      overflow
-    ));
+    const time = calendar.timeFromFields(fields, options, GetIntrinsic('%Temporal.Time%'));


Suggested change

const time = calendar.timeFromFields(fields, options, GetIntrinsic('%Temporal.Time%'));

const time = calendar.timeFromFields(fields, { overflow }, GetIntrinsic('%Temporal.Time%'));

I think we are normally creating a new options object to pass to the calendar methods instead of passing through the user-supplied one.

@ptomato check the dateFromFields call right above this. It passes in options directly. I spotted a lot of such issues early-on and even made a few refactor commits that I later dropped to keep this PR as small and on-topic as possible. Should I fix all sites in this PR, or would a single follow-up that takes care of all such cases suffice?

ptomato · 2020-10-30T22:40:37Z

polyfill/lib/datetime.mjs

-      GetSlot(other, MICROSECOND),
-      GetSlot(other, NANOSECOND)
+    let { days: deltaDays, hours, minutes, seconds, milliseconds, microseconds, nanoseconds } = calendar.timeUntil(
+      this.toTime(),


These should be abstract operations so that the call to toTime isn't observable (though feel free to defer this to your next PR)

ptomato · 2020-10-30T22:43:27Z

polyfill/lib/ecmascript.mjs

+  ConsolidateCalendars: (one, two) => {
+    const sOne = ES.CalendarToString(one);
+    const sTwo = ES.CalendarToString(two);
+    if (sOne === sTwo || sOne === 'iso8601') {


I thought we had removed the special handling of the ISO calendar as a "null" calendar everywhere else — double check with @sffc who probably remembers better than I do.

I'll reach out to @sffc about this, thanks.

polyfill/lib/time.mjs

polyfill/lib/zoneddatetime.mjs

ptomato · 2020-10-30T22:52:57Z

polyfill/lib/time.mjs

-      GetSlot(other, MICROSECOND),
-      GetSlot(other, NANOSECOND)
-    );
+    let { hours, minutes, seconds, milliseconds, microseconds, nanoseconds } = calendar.timeUntil(this, other);
    ({ hours, minutes, seconds, milliseconds, microseconds, nanoseconds } = ES.RoundDuration(


This is going to be a lot of work, but I believe RoundDuration will need to round according to the calendar's length of hours and minutes? It's fine to not block this PR on it.

Well, wasn't the plan to keep Durations ISO-only? But I think I get your point more or less. Since Time.p.add(duration) interprets the duration in a calendar-specific manner, Time.p.until should produce a Duration based on the calendar. Let's make an issue to discuss this more in detail and land this? We can either generalize this as calendar.durationRound or just add the logic to the until/since methods on the calendar.

polyfill/test/usercalendar.mjs

ryzokuken · 2020-11-02T04:48:52Z

IIUC, all outstanding comments have been addressed and this should be ready to merge. However, a couple of things need to be worked out in follow-ups. In the meantime, I'd also start working on the spec text and docs for this change. Thanks everyone!

Ms2ger · 2020-11-02T12:41:26Z

Travis is having issues, so I tested locally before merging.

ryzokuken self-assigned this Oct 27, 2020

ryzokuken requested a review from ptomato October 27, 2020 14:50

ryzokuken added the non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! label Oct 27, 2020

ryzokuken added this to the Stable proposal milestone Oct 27, 2020

ptomato reviewed Oct 27, 2020

View reviewed changes

ptomato requested changes Oct 27, 2020

View reviewed changes

polyfill/lib/calendar.mjs Outdated Show resolved Hide resolved

polyfill/lib/calendar.mjs Show resolved Hide resolved

polyfill/lib/datetime.mjs Outdated Show resolved Hide resolved

justingrant reviewed Oct 27, 2020

View reviewed changes

Ms2ger added the awaiting-author label Oct 28, 2020

ryzokuken force-pushed the calendar/time branch from 6e499a9 to 806984a Compare October 28, 2020 09:56

ptomato reviewed Oct 29, 2020

View reviewed changes

polyfill/lib/datetime.mjs Show resolved Hide resolved

ryzokuken force-pushed the calendar/time branch from 597603b to 185aa2c Compare October 29, 2020 11:04

ryzokuken force-pushed the calendar/time branch from e75798c to f53e812 Compare October 30, 2020 19:51

ptomato approved these changes Oct 30, 2020

View reviewed changes

polyfill: make calendar undefined checks more consistent

1140b1e

ryzokuken force-pushed the calendar/time branch from 240482b to fc5fdf2 Compare November 2, 2020 12:14

ryzokuken added 2 commits November 2, 2020 17:59

polyfill: introduce calendar systems for time

0861f6b

polyfill: add tests for Time.p.getISOFields

8e4de7b

ryzokuken force-pushed the calendar/time branch from fc5fdf2 to 8e4de7b Compare November 2, 2020 12:30

Ms2ger merged commit 80b97e3 into tc39:main Nov 2, 2020

Ms2ger removed the awaiting-author label Nov 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce calendar systems to time #1062

Introduce calendar systems to time #1062

ryzokuken commented Oct 27, 2020

ptomato left a comment

ptomato left a comment

justingrant Oct 27, 2020

justingrant Oct 28, 2020

ptomato Oct 28, 2020

justingrant Oct 27, 2020

ptomato Oct 28, 2020

justingrant commented Oct 28, 2020

codecov bot commented Oct 29, 2020 •

edited

Loading

ptomato left a comment

ptomato Oct 30, 2020

ryzokuken Nov 2, 2020

ptomato Oct 30, 2020

ryzokuken Nov 2, 2020

ptomato Oct 30, 2020

ryzokuken Nov 2, 2020

ptomato Oct 30, 2020

ryzokuken Nov 2, 2020

ryzokuken commented Nov 2, 2020

Ms2ger commented Nov 2, 2020

	const time = calendar.timeFromFields(fields, options, GetIntrinsic('%Temporal.Time%'));
	const time = calendar.timeFromFields(fields, { overflow }, GetIntrinsic('%Temporal.Time%'));

Introduce calendar systems to time #1062

Introduce calendar systems to time #1062

Conversation

ryzokuken commented Oct 27, 2020

ptomato left a comment

Choose a reason for hiding this comment

ptomato 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

justingrant commented Oct 28, 2020

codecov bot commented Oct 29, 2020 • edited Loading

Codecov Report

ptomato 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

ryzokuken commented Nov 2, 2020

Ms2ger commented Nov 2, 2020

codecov bot commented Oct 29, 2020 •

edited

Loading