Skip to content
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

Comparison and equality of dates with different calendars #625

Closed
ptomato opened this issue May 26, 2020 · 9 comments
Closed

Comparison and equality of dates with different calendars #625

ptomato opened this issue May 26, 2020 · 9 comments
Labels
calendar Part of the effort for Temporal Calendar API

Comments

@ptomato
Copy link
Collaborator

ptomato commented May 26, 2020

Follow up from #588.

When we add calendar slots to Temporal.DateTime, Temporal.Date, and Temporal.YearMonth, there's the question of whether two dates with different calendars but referring to the same day should be considered equal.

For example, these are all the same day:

  • Temporal.Date.from({ year: 2020, month: 5, day: 22, calendar: 'iso8601' })
  • Temporal.Date.from({ year: 2020, month: 5, day: 22, calendar: 'gregory' })
  • Temporal.Date.from({ year: 5780, month: 8, day: 28, calendar: 'hebrew' })
  • Temporal.Date.from({ year: 1441, month: 9, day: 29, calendar: 'islamic' })
  • Temporal.Date.from({ era: 'reiwa', year: 2, month: 5, day: 22, calendar: 'japanese' })

Temporal API users may want two different kinds of "equality":

  • "same day" — two objects refer to the same day but the calendar may be different
  • "object identity" ­— two objects have exactly the same internal state, i.e. don't ignore the calendar

It's unavoidable to prioritize one of these use cases above the other.

On #588 several ideas were proposed:

  1. Prioritize "same day". Lexicographical ordering of calendars by ID is a way to sort calendars, but it's not the only way, so it's better to make the user make that choice. If a user wants "object identity", then they specify their own way to sort calendars like this:
  • date1.equals(date2) && date1.calendar.id === date2.calendar.id
  • dates.sort((d1, d2) => Temporal.Date.compare(d1, d2) || d1.calendar.id <=> d2.calendar.id) (spaceship operator left as an exercise for the reader)
  1. Prioritize "object identity". Compare/sort the date ISO fields first and then the calendar ID last. Lexicographical ordering of calendars by ID is a sensible default and it's reasonable to write something from scratch if you want something different than that, and furthermore it's also consistent with how the corresponding ISO strings with [c=...] annotation would be sorted. If a user wants "same day", then they work around it like this:
  • date1.withCalendar('iso8601').equals(date2.withCalendar('iso8601'))
  • dates.sort((d1, d2) => Temporal.Date.compare(d1.withCalendar('iso8601'), d2.withCalendar('iso8601'))
  1. Try to mitigate the difference in convenience between the two use cases in some way, for example:
  • prioritize "object identity", but make compare() return ±2 if the dates differ, and ±1 if only the calendars differ but the dates are the same.
  • add an option to compare() and equals() such as { ignoreCalendar: true }, similar to Intl.Collator(..., { sensitivity: 'base' }).

Temporal.MonthDay is different, since it's not possible in the first place to compare two MonthDays from different calendars, which is why we removed Temporal.MonthDay.compare(). So I believe Temporal.MonthDay.equals() needs to do "object identity" in any case, which suggests that option 1 would make things inconsistent.

@ptomato ptomato added the calendar Part of the effort for Temporal Calendar API label May 26, 2020
@ptomato ptomato added this to the Stage 3 milestone May 26, 2020
@ptomato
Copy link
Collaborator Author

ptomato commented May 26, 2020

(I suggest we go with option 2 for the initial release of the polyfill, since it's the simplest)

@justingrant
Copy link
Collaborator

Defaulting to "same day" means that date.day!==other.day and date.equals(other) could both be true, which seems very unexpected and a potential foot gun. So I'd argue against making that the default.

But I can also see how "object identity" would also be surprising for some developers.

Another option would be for the comparison to throw because there's not necessarily an obvious default behavior that won't cause problems. Per our defaults discussion, non-ISO calendars are an opt-in API anyways, so requiring an explicit opt-in for cross-calendar comparison too seems reasonable.

For overriding default behavior and choosing to compare using ISO values only, the Intl example of a collator feels like the right pattern-- default to "object equality" but allow users to get a comparer function that behaves they way they want.

If we did throw, it'd be nice if the error message was something that provided a hint for how the developer could solve the problem, e.g. Invalid cross-calendar comparison. Use Temporal.Calendar.collator instead.

@ptomato ptomato modified the milestones: Stage 3, Stable API Jun 4, 2020
ptomato added a commit that referenced this issue Jun 5, 2020
This adds an 'iso8601' calendar and changes the polyfill code for
Temporal.Date, Temporal.DateTime, Temporal.YearMonth, and
Temporal.MonthDay so that they gain a CALENDAR slot, and delegate all of
their calendar-sensitive operations to the object in the CALENDAR slot.

Among the changes made to those types:
- Constructors gain a calendar argument of type Temporal.Calendar.
- Types gain a 'calendar' property of type Temporal.Calendar.
- from() accepts a 'calendar' string or Temporal.Calendar.
- Temporal.Date and Temporal.DateTime accept a calendar in with().
- Temporal.MonthDay & Temporal.YearMonth throw on a calendar in with().
- The calendar is returned in getFields() (see #641).
- equals() and compare() take the calendar into account (see #625).

This includes documentation, but leaves out spec changes and all but the
necessary test changes for the time being.
ptomato added a commit that referenced this issue Jun 5, 2020
This adds an 'iso8601' calendar and changes the polyfill code for
Temporal.Date, Temporal.DateTime, Temporal.YearMonth, and
Temporal.MonthDay so that they gain a CALENDAR slot, and delegate all of
their calendar-sensitive operations to the object in the CALENDAR slot.

Among the changes made to those types:
- Constructors gain a calendar argument of type Temporal.Calendar.
- Types gain a 'calendar' property of type Temporal.Calendar.
- from() accepts a 'calendar' string or Temporal.Calendar.
- Temporal.Date and Temporal.DateTime accept a calendar in with().
- Temporal.MonthDay & Temporal.YearMonth throw on a calendar in with().
- The calendar is returned in getFields() (see #641).
- equals() and compare() take the calendar into account (see #625).

This includes documentation, but leaves out spec changes and all but the
necessary test changes for the time being.
ptomato added a commit that referenced this issue Jun 9, 2020
This adds an 'iso8601' calendar and changes the polyfill code for
Temporal.Date, Temporal.DateTime, Temporal.YearMonth, and
Temporal.MonthDay so that they gain a CALENDAR slot, and delegate all of
their calendar-sensitive operations to the object in the CALENDAR slot.

Among the changes made to those types:
- Constructors gain a calendar argument of type Temporal.Calendar.
- Types gain a 'calendar' property of type Temporal.Calendar.
- from() accepts a 'calendar' string or Temporal.Calendar.
- Temporal.Date and Temporal.DateTime accept a calendar in with().
- Temporal.MonthDay & Temporal.YearMonth throw on a calendar in with().
- The calendar is returned in getFields() (see #641).
- equals() and compare() take the calendar into account (see #625).

This includes documentation, but leaves out spec changes and all but the
necessary test changes for the time being.
ptomato added a commit that referenced this issue Jun 9, 2020
This adds an 'iso8601' calendar and changes the polyfill code for
Temporal.Date, Temporal.DateTime, Temporal.YearMonth, and
Temporal.MonthDay so that they gain a CALENDAR slot, and delegate all of
their calendar-sensitive operations to the object in the CALENDAR slot.

Among the changes made to those types:
- Constructors gain a calendar argument of type Temporal.Calendar.
- Types gain a 'calendar' property of type Temporal.Calendar.
- from() accepts a 'calendar' string or Temporal.Calendar.
- Temporal.Date and Temporal.DateTime accept a calendar in with().
- Temporal.MonthDay & Temporal.YearMonth throw on a calendar in with().
- The calendar is returned in getFields() (see #641).
- equals() and compare() take the calendar into account (see #625).

This includes documentation, but leaves out spec changes and all but the
necessary test changes for the time being.
ptomato added a commit that referenced this issue Jun 10, 2020
This adds an 'iso8601' calendar and changes the polyfill code for
Temporal.Date, Temporal.DateTime, Temporal.YearMonth, and
Temporal.MonthDay so that they gain a CALENDAR slot, and delegate all of
their calendar-sensitive operations to the object in the CALENDAR slot.

Among the changes made to those types:
- Constructors gain a calendar argument of type Temporal.Calendar.
- Types gain a 'calendar' property of type Temporal.Calendar.
- from() accepts a 'calendar' string or Temporal.Calendar.
- Temporal.Date and Temporal.DateTime accept a calendar in with().
- Temporal.MonthDay & Temporal.YearMonth throw on a calendar in with().
- The calendar is returned in getFields() (see #641).
- equals() and compare() take the calendar into account (see #625).

This includes documentation, but leaves out spec changes and all but the
necessary test changes for the time being.
ptomato added a commit that referenced this issue Jun 12, 2020
This adds an 'iso8601' calendar and changes the polyfill code for
Temporal.Date, Temporal.DateTime, Temporal.YearMonth, and
Temporal.MonthDay so that they gain a CALENDAR slot, and delegate all of
their calendar-sensitive operations to the object in the CALENDAR slot.

Among the changes made to those types:
- Constructors gain a calendar argument of type Temporal.Calendar.
- Types gain a 'calendar' property of type Temporal.Calendar.
- from() accepts a 'calendar' string or Temporal.Calendar.
- Temporal.Date and Temporal.DateTime accept a calendar in with().
- Temporal.MonthDay & Temporal.YearMonth throw on a calendar in with().
- The calendar is returned in getFields() (see #641).
- equals() and compare() take the calendar into account (see #625).

This includes documentation, but leaves out spec changes and all but the
necessary test changes for the time being.
@sffc
Copy link
Collaborator

sffc commented Sep 2, 2020

First choice: object equality with ±2 and ±1 on the compare method.

Second choice: object equality with only ±1 on the compare method.

Third choice: same-day equality with ±2 and ±1 on the compare method.

Fourth choice: anything that requires explicitly pulling out date.calendar.id and comparing it if you care about differences in the calendar system.

@justingrant
Copy link
Collaborator

What about continuing the pattern of getISOCalendarFields? For example, compareISOCalendar could provide a same-day method, and compare would use object identity?

±2 feels very undiscoverable.

@ptomato
Copy link
Collaborator Author

ptomato commented Sep 2, 2020

±2 feels very undiscoverable.

+2 to that.

The way I would expect it to work if I didn't know better, would still be same-day equality, but the problems that others have mentioned have convinced me that it's not the way to go. On balance I think I prefer the status quo (object equality with only ±1 on the compare method.)

@justingrant
Copy link
Collaborator

What about continuing the pattern of getISOCalendarFields? For example, compareISOCalendar could provide a same-day method, and compare would use object identity?

BTW, another potential advantage of this split would be discoverability, esp. for IDE users. (as long as the same-day method name starts with "compare")

So my preference would be:

  1. Two methods for comparison (with the same-day method name starting with "compare" plus some suffix, e.g. compareISOCalendar), and object-equality only for equals.
  2. Status quo: object equality with only ±1 on the compare method

@justingrant
Copy link
Collaborator

justingrant commented Sep 11, 2020

Decision 2020-09-11: add an equalsISO method that ignores the calendar (just compare the ISO slots).

@ptomato
Copy link
Collaborator Author

ptomato commented Sep 17, 2020

(Or maybe not, pending #912.)

@ptomato
Copy link
Collaborator Author

ptomato commented Sep 18, 2020

Meeting, Sept. 18: We're reversing the decision from last week, no equalsISO method. This can be added to Temporal in a future proposal if people are using it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
calendar Part of the effort for Temporal Calendar API
Projects
None yet
Development

No branches or pull requests

3 participants