tc39 / proposal-temporal

Provides standard objects and functions for working with dates and times.
https://tc39.es/proposal-temporal/docs/
Other
3.36k stars 155 forks source link

Calendar-dependent compare() for Temporal.MonthDay? #523

Closed ptomato closed 4 years ago

ptomato commented 4 years ago

The Calendar draft document doesn't delegate compare() methods to the calendar because in most cases it doesn't seem necessary. You can use the ISO-valued internal slots because no matter what calendar a date is projected into, it doesn't change whether it is before, after, or equal to another date.

However, with the data model adopted in #391, this doesn't necessarily hold for Temporal.MonthDay anymore. It still does hold in the case where the calendar can assign one RefIsoYear to all its MonthDay instances (as the Gregorian calendar does, where we can pick any leap year as RefIsoYear such as 1972 and as long as we always use the same one, comparisons will still work.) But in lunar calendars, such as Hebrew, it won't work.

On the other hand, if we tried to fix this by reading the fields with Get, then that wouldn't work for calendars such as Japanese with eras (where 1 Reiwa comes after 31 Heisei, you'd have to know to take the era into account) so then all comparisons would have to be delegated to the calendar.

Is it better to delegate all comparisons to the calendar (more burden on calendar implementors), or only Temporal.MonthDay.compare() (making things inconsistent)?

Note: currently in the spec, comparisons read internal slots, whereas in the polyfill they perform a Get. So this is inconsistent and needs to be resolved either way.

ljharb commented 4 years ago

It should definitely be delegated to the calendar. It can also have a default implementation so that most calendar implementors don't need to care about it.

Ms2ger commented 4 years ago

It should definitely be delegated to the calendar. It can also have a default implementation so that most calendar implementors don't need to care about it.

There seems to be a "because…" part missing here.

ryzokuken commented 4 years ago

@ptomato should we discuss this in greater detail in the meeting?

sffc commented 4 years ago

The easiest option would be to establish the convention that non-Gregorian calendars need to pick refYears that cause MonthDay orders to be correct.

littledan commented 4 years ago

Do we need comparison for MonthDay?

ptomato commented 4 years ago

I would be fine to remove comparison from MonthDay. It's cyclical anyway (as is Time, for that matter!) so is it really true that 12-31 > 01-01?

(Although, Temporal.Time.compare is used in https://tc39.es/proposal-temporal/docs/cookbook.html#comparison-of-an-instant-to-business-hours)

Removing MonthDay comparison would be in keeping with removing MonthDay arithmetic, which we've already done, and all other types are comparable by their ISO fields so wouldn't need to delegate to the calendar.

sffc commented 4 years ago

Sounds good to me. A future proposal can always add this method.

ptomato commented 4 years ago

While removing this I noticed that we do have a legitimate use of it in the tests, to check whether two MonthDays are equal. Maybe we should add Temporal.MonthDay.equal() instead, which would still compare by ISO fields / internal slots and return false if the two operands had different calendars?

ljharb commented 4 years ago

compare could still return 0 on MonthDay if they're equal, and NaN if they're not.

ptomato commented 4 years ago

Would that preclude a proper compare method from being added in a future proposal?

sffc commented 4 years ago

+1 on adding MonthDay.prototype.equal. We should add that to all of the types.

As an alternative, you can always do md1.toString() === md2.toString(), or check for getFields() equality.

ptomato commented 4 years ago

It turns out that we do a lot of this sort of thing in the tests:

equal(`${date1.someOperation()}`, `${date2}`)

So I think there is a well-demonstrated use case for an equal() method.

sffc commented 4 years ago

There is a difference between .compare() and .equals(): the latter should also check for calendar system equality. In other words,

const d1 = Temporal.Date.from("2020-05-18");
const d2 = d1.withCalendar("japanese");
assertEquals(0, d1.compare(d2));
assertFalse(d1.equals(d2));

This is another reason to include .equals(), since it can't be perfectly modeled by .compare() == 0.

ptomato commented 4 years ago

Do you think that would make it confusing to add Temporal.Absolute.prototype.equals since unlike the other equals methods, it wouldn't check for calendar equality?