Editorial: Use ISO Date, ISO Date-Time, and Time Records in more places #3003

ptomato · 2024-10-08T01:31:52Z

This has been on the to-do list since removing user-visible calls from calendars and time zones. It's a huge improvement in conciseness and therefore readability and comprehensibility; and none of it is observable.

Highlights:

ISO Date-Time Record now composed of [[ISODate]] (ISO Date Record) and [[Time]] (Time Record) fields
Abstract operations that previously took a series of date/time components (year, month, day, hour, etc.) now take one of the three Records
Temporal.PlainDate, PlainDateTime, PlainYearMonth, PlainMonthDay, and PlainTime's internal slots that each stored a date or time component ([[Year]], [[Month]], etc.) are now collapsed into one; they store one of the three Records

Could be reviewed commit by commit, but it's probably just as easy to review in one pass.

Closes: #2949, #3000

We test for the Time Record being ~start-of-day~ here, but that cannot happen; InterpretTemporalDateTimeFields cannot return that value. Closes: #3000

ISOMonthCode was only used in CalendarISOToDate and in an assertion which didn't really need to be there. See: #2948

Instead of passing each component individually. See: #2949

codecov · 2024-10-08T01:34:15Z

Codecov Report

Attention: Patch coverage is 97.23577% with 17 lines in your changes missing coverage. Please review.

Project coverage is 95.88%. Comparing base (b0e9b3a) to head (3490b8f).
Report is 30 commits behind head on main.

Files with missing lines	Patch %	Lines
polyfill/lib/ecmascript.mjs	98.05%	6 Missing and 1 partial ⚠️
polyfill/lib/plaintime.mjs	75.00%	1 Missing and 5 partials ⚠️
polyfill/lib/calendar.mjs	75.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3003      +/-   ##
==========================================
- Coverage   96.36%   95.88%   -0.49%     
==========================================
  Files          21       21              
  Lines       10491     9605     -886     
  Branches     1821     1793      -28     
==========================================
- Hits        10110     9210     -900     
- Misses        323      342      +19     
+ Partials       58       53       -5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

In CompareISODate, use two ISO Date Records instead of passing each component individually. In ISODateSurpasses, the first date does not have to exist, so rewrite the operation without using CompareISODate and only take an ISO Date Record for the second parameter. See: #2949

Instead of passing each component individually. See: #2949

justingrant

I love this PR! Thanks so much for building this. Makes things much easier to read. I was able to get through only the polyfill changes before I ran out of time, so I'll leave the spec review parts for others.

My comments were mainly suggesting other AOs that might want to take records.

justingrant · 2024-10-08T04:30:07Z

polyfill/lib/ecmascript.mjs

@@ -1545,9 +1413,9 @@ export function InterpretISODateTimeOffset(
  if (offsetBehaviour === 'exact' || offsetOpt === 'use') {
    // Calculate the instant for the input's date/time and offset
    const balanced = BalanceISODateTime(


Why does BalanceISODateTime take individual units instead of a date and time record?

There are a number of operations such as BalanceISODateTime, IsValidISODate, RejectTime, etc. that I left alone, that still take individual components. The reason is that these operations can accept unconstrained values. (For example, BalanceISODateTime(1970, 1, 1, 0, -60, 0, 0, 0, 0) when we are subtracting the offsetMinutes from the +01:00 time zone.) Currently, it's an invariant that the ISO Date/Date-Time/Time Records can only contain valid constrained values.

An alternative would be to allow the records to contain unconstrained values, then these AOs could also take records. But on the downside, you then have to have checks or assertions everywhere you use them. On balance I found it better to have the invariant, and write out the components when the values may be unconstrained.

Ahh, makes sense. I should have known there was a good reason!

justingrant · 2024-10-08T04:41:21Z

polyfill/lib/ecmascript.mjs

    ValidateEpochNanoseconds(epochNs);
    return epochNs;
  }

-  if (MathAbs(ISODateToEpochDays(year, month - 1, day)) > 1e8) {
+  if (MathAbs(ISODateToEpochDays(isoDate.year, isoDate.month - 1, isoDate.day)) > 1e8) {


ISODateToEpochDays is referenced 11 times, and 9 of those are deconstructing an existing ISO date object into three arguments. And in all 11 cases, the month is decremented by 1 before calling the function.

Maybe refactor this AO to accept a single date argument, and that handles the decrement internally?

Also, out of curiosity, why is the decrement needed?

Also, the > 1e8 validation is repeated 4 times. Should it be abstracted to a new AO, e.g. ValidateISODateRange(date) ?

ISODateToEpochDays is supposed to be a 1-to-1 replacement for ECMA-262 MakeDay but using mathematical values, because some of the ECMA-262 operations related to MakeDay don't correctly handle the edges of the time value range. There is an issue open for ECMA-262 to fix this, and when it's fixed the intention is to go back to using MakeDay here.

MakeDay and the other existing ECMA-262 ops use 0-based month numbers, so ISODateToEpochDays does as well. That's why we have the decrement. We can maybe improve this down the line when Temporal reaches stage 4.

About the "abs(ISODateToEpochDays(year, month - 1, day)) > 1e8" pattern, it's there because of #2729 and #2985. I like the idea of abstracting it, especially because it can disappear when after that ECMA-262 issue is fixed.

justingrant · 2024-10-08T04:53:46Z

polyfill/lib/ecmascript.mjs

-      millisecond,
-      microsecond,
-      nanosecond
+      isoDateTime.isoDate.year,


Out of curiosity why wasn't this refactored to something like this?

BalanceISODateTime({ ...isoDateTime, minute: isoDateTime.time.minute - offsetMinutes })

This is a great suggestion. I just didn't think of it. Will do as part of #3005.

Never mind, running on little sleep and didn't see the curly brackets 😄 In that case, same as my earlier comment, I think BalanceISODateTime should continue to take separate components.

polyfill/lib/ecmascript.mjs

polyfill/lib/plaindatetime.mjs

justingrant · 2024-10-08T05:36:28Z

polyfill/lib/plaindatetime.mjs

-    fields.nanosecond = GetSlot(this, ISO_NANOSECOND);
+    let fields = ES.ISODateToFields(calendar, GetSlot(this, ISO_DATE_TIME).isoDate);
+    const isoDateTime = GetSlot(this, ISO_DATE_TIME);
+    fields.hour = isoDateTime.time.hour;


Should this befields = { ...fields, ...isoDateTime.time } ?

polyfill/lib/plaintime.mjs

justingrant · 2024-10-08T05:39:30Z

polyfill/lib/zoneddatetime.mjs

-    fields.microsecond = isoDateTime.microsecond;
-    fields.nanosecond = isoDateTime.nanosecond;
+    let fields = ES.ISODateToFields(calendar, isoDateTime.isoDate);
+    fields.hour = isoDateTime.time.hour;


Should this befields = { ...fields, ...isoDateTime.time } ?

Ms2ger

Thanks! This PR is already large enough that I'm going to defer @justingrant's suggestions to another one and only fix the typos here.

polyfill/lib/plaintime.mjs

spec/plaintime.html

Ms2ger · 2024-10-08T08:22:00Z

polyfill/lib/ecmascript.mjs

-  return ToTemporalTime(item);
+export function ToTimeRecordOrMidnight(item) {
+  if (item === undefined) return MidnightTimeRecord();
+  return GetSlot(ToTemporalTime(item), TIME);


Future work: it seems like ToTemporalTime could also return a record

I did look at doing that, but I think I still prefer what it currently does; it's analogous with ToTemporalDate, ToTemporalZonedDateTime, etc. In this one case it'd be better to return a record, but I'm not sure if it'd be better elsewhere.

Instead of passing each component individually, pass a Time Record. The name is no longer really appropriate, since we are not passing a Temporal object, so rename it to TimeRecordToString. See: #2949 Co-authored-by: Ms2ger <[email protected]>

Instead of passing each component individually, pass Time Records. The name is no longer really appropriate, since we are not passing a Temporal object, so rename it to CompareTimeRecord. See: #2949

Instead of passing each component individually. See: #2949

…rename Instead of passing each component individually, pass an ISO Date-Time Record. The name is no longer really appropriate, since we are not passing a Temporal object, so rename it to ISODateTimeToString. See: #2949

…ecord Instead of individual fields, give ISO Date-Time Record just two fields, [[ISODate]] and [[Time]]. Since we are passing ISO Date and Time Records in more places, this is better for readability. See: #2949

Instead of passing each component individually. See: #2949

…nding Instead of passing each component individually. See: #2949

Instead of passing each component individually. See: #2949

Instead of passing each component individually. The [[Day]] field is ignored. See: #2949

Instead of passing each component individually. GetUTCEpochNanoseconds is part of ECMA-262 already, so this requires some <ins> and <del> tags. See: #2949

Instead of passing each component individually. GetNamedTimeZoneEpochNanoseconds is part of ECMA-262 already, so this requires some <ins> and <del> tags. See: #2949

These are repeated several times now throughout the spec, so abstract them out for readability. See: #2949

For PlainDate, PlainYearMonth, and PlainMonthDay, use an ISO Date Record in the [[ISODate]] internal slot. For PlainDateTime, use an ISO Date-Time Record in the [[ISODateTime]] internal slot. For PlainTime, use a Time Record in the [[Time]] internal slot. This makes a lot more concise spec text, and probably corresponds better with how it will be implemented. See: #2949 Co-authored-by: Justin Grant <[email protected]> Co-authored-by: Ms2ger <[email protected]>

Instead of writing out all the field names, add an abstract operation that also encapsulates the IsValidTime assertion. See: #2949 Co-authored-by: Ms2ger <[email protected]>

This is the only place it's called from, and it consolidates operations that take individual time components. See: #2949

This avoids creation of an unobservable Temporal.PlainDateTime object, and is also simpler to read. See: #2949

This fits with the general theme of returning Records instead of Objects, but also these operations were each only called from one place. So inline them. See: #2949

Now that it's trivial to get an ISO Date Record for a Temporal object, we don't really need this operation. All the switching on Temporal object type that it does, is already known at each call site.

This avoids creation of an unobservable Temporal.PlainTime object, and is also simpler to read. Since it no longer returns a Temporal object, give it a more appropriate name.

Instead of passing each component individually. Also, remove the "To" from the name, as it's not a conversion operation. See: #2949

ptomato added 3 commits October 7, 2024 11:42

Editorial: Remove dead code from PlainDateTime.with

4cc5eb3

We test for the Time Record being ~start-of-day~ here, but that cannot happen; InterpretTemporalDateTimeFields cannot return that value. Closes: #3000

Editorial: Inline ISOMonthCode into CalendarISOToDate

7798f53

ISOMonthCode was only used in CalendarISOToDate and in an assertion which didn't really need to be there. See: #2948

Editorial: Use ISO Date(-Time) Record in ISODate(Time)WithinLimits

29d9355

Instead of passing each component individually. See: #2949

ptomato requested review from Ms2ger, justingrant and gibson042 October 8, 2024 01:31

ptomato mentioned this pull request Oct 8, 2024

Final editorial spec text plan #2950

Closed

27 tasks

ptomato added 2 commits October 7, 2024 18:38

Editorial: Use Time Record in DifferenceTime

0387343

Instead of passing each component individually. See: #2949

ptomato force-pushed the 2949-iso-date-time-records branch from 20e0b3d to af41492 Compare October 8, 2024 01:39

justingrant approved these changes Oct 8, 2024

View reviewed changes

Ms2ger approved these changes Oct 8, 2024

View reviewed changes

ptomato and others added 17 commits October 8, 2024 10:47

Editorial: Use Time Record in CompareTemporalTime, and rename

811ff66

Instead of passing each component individually, pass Time Records. The name is no longer really appropriate, since we are not passing a Temporal object, so rename it to CompareTimeRecord. See: #2949

Editorial: Use Time Record in AddTime

15f6617

Instead of passing each component individually. See: #2949

Editorial: Use ISO Date-Time Record in RoundISODateTime

a35c5bb

Instead of passing each component individually. See: #2949

Editorial: Use Time Record in RoundTime

3ca5d40

Instead of passing each component individually. See: #2949

Editorial: Compose ISO Date-Time Record of ISO Date Record and Time R…

e61edc2

…ecord Instead of individual fields, give ISO Date-Time Record just two fields, [[ISODate]] and [[Time]]. Since we are passing ISO Date and Time Records in more places, this is better for readability. See: #2949

Editorial: Use ISO Date-Time Record in CompareISODateTime

a8db2d8

Instead of passing each component individually. See: #2949

Editorial: Use ISO Date-Time Record in DifferenceISODateTime

40a21c9

Instead of passing each component individually. See: #2949

Editorial: Use ISO Date-Time Record in DifferencePlainDateTimeWithRou…

6812a1c

…nding Instead of passing each component individually. See: #2949

Editorial: Use ISO Date-Time Record in DifferencePlainDateTimeWithTotal

021f74b

Instead of passing each component individually. See: #2949

Editorial: Use ISO Date Record in InterpretISODateTimeOffset

9180d74

Instead of passing each component individually. See: #2949

Editorial: Use ISO Date Record in ISOYearMonthWithinLimits

69f8d7a

Instead of passing each component individually. The [[Day]] field is ignored. See: #2949

Editorial: Use ISO Date-Time Record in GetUTCEpochNanoseconds

148cf89

Instead of passing each component individually. GetUTCEpochNanoseconds is part of ECMA-262 already, so this requires some <ins> and <del> tags. See: #2949

Editorial: Use ISO Date-Time Record in GetNamedTimeZoneEpochNanoseconds

0a1e7ef

Instead of passing each component individually. GetNamedTimeZoneEpochNanoseconds is part of ECMA-262 already, so this requires some <ins> and <del> tags. See: #2949

Editorial: Add MidnightTimeRecord and NoonTimeRecord

50923de

These are repeated several times now throughout the spec, so abstract them out for readability. See: #2949

ptomato and others added 8 commits October 8, 2024 10:49

Editorial: Add CreateTimeRecord AO

287c79b

Instead of writing out all the field names, add an abstract operation that also encapsulates the IsValidTime assertion. See: #2949 Co-authored-by: Ms2ger <[email protected]>

Editorial: Inline ConstrainTime into RegulateTime

1fcb24d

This is the only place it's called from, and it consolidates operations that take individual time components. See: #2949

Editorial: Return ISO Date-Time Record from SystemDateTime

1eb5ab7

This avoids creation of an unobservable Temporal.PlainDateTime object, and is also simpler to read. See: #2949

Editorial: Inline SystemInstant and SystemZonedDateTime

e76e417

This fits with the general theme of returning Records instead of Objects, but also these operations were each only called from one place. So inline them. See: #2949

Editorial: Remove TemporalObjectToFields

c957b24

Now that it's trivial to get an ISO Date Record for a Temporal object, we don't really need this operation. All the switching on Temporal object type that it does, is already known at each call site.

Editorial: Remove duplicated line in RegulateISODate

4ae4c4a

Editorial: Return Time Record from ToTemporalTimeOrMidnight, and rename

f9bce00

This avoids creation of an unobservable Temporal.PlainTime object, and is also simpler to read. Since it no longer returns a Temporal object, give it a more appropriate name.

Editorial: Use ISO Date Records in ToISOWeekOfYear/DayOfYear/DayOfWeek

3490b8f

Instead of passing each component individually. Also, remove the "To" from the name, as it's not a conversion operation. See: #2949

Ms2ger force-pushed the 2949-iso-date-time-records branch from e5ab7a2 to 3490b8f Compare October 8, 2024 08:50

Ms2ger mentioned this pull request Oct 8, 2024

Use ISO Date, ISO Date-Time, and Time Records in more places in the polyfill #3005

Closed

Ms2ger merged commit cb2e0ff into main Oct 8, 2024
6 checks passed

Ms2ger deleted the 2949-iso-date-time-records branch October 8, 2024 08:52

ptomato mentioned this pull request Oct 8, 2024

Editorial: Follow up suggestions from Justin #3006

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Editorial: Use ISO Date, ISO Date-Time, and Time Records in more places #3003

Editorial: Use ISO Date, ISO Date-Time, and Time Records in more places #3003

ptomato commented Oct 8, 2024 •

edited

Loading

codecov bot commented Oct 8, 2024 •

edited

Loading

justingrant left a comment

justingrant Oct 8, 2024

ptomato Oct 8, 2024

justingrant Oct 8, 2024

justingrant Oct 8, 2024 •

edited

Loading

ptomato Oct 8, 2024

justingrant Oct 8, 2024

ptomato Oct 8, 2024

ptomato Oct 8, 2024

justingrant Oct 8, 2024

justingrant Oct 8, 2024

Ms2ger left a comment

Ms2ger Oct 8, 2024

ptomato Oct 8, 2024

Editorial: Use ISO Date, ISO Date-Time, and Time Records in more places #3003

Editorial: Use ISO Date, ISO Date-Time, and Time Records in more places #3003

Conversation

ptomato commented Oct 8, 2024 • edited Loading

codecov bot commented Oct 8, 2024 • edited Loading

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

justingrant Oct 8, 2024 • edited Loading

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 Oct 8, 2024 •

edited

Loading

codecov bot commented Oct 8, 2024 •

edited

Loading

justingrant Oct 8, 2024 •

edited

Loading