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

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

Closed
ptomato opened this issue Apr 22, 2020 · 14 comments · Fixed by #588
Closed

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

ptomato opened this issue Apr 22, 2020 · 14 comments · Fixed by #588
Labels
calendar Part of the effort for Temporal Calendar API

Comments

@ptomato
Copy link
Collaborator

ptomato commented Apr 22, 2020

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.

@ptomato ptomato added the calendar Part of the effort for Temporal Calendar API label Apr 22, 2020
@ljharb
Copy link
Member

ljharb commented Apr 23, 2020

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
Copy link
Collaborator

Ms2ger commented Apr 23, 2020

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
Copy link
Member

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

@sffc
Copy link
Collaborator

sffc commented Apr 30, 2020

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
Copy link
Member

Do we need comparison for MonthDay?

@littledan littledan added this to the Stage 3 milestone May 14, 2020
@ptomato
Copy link
Collaborator Author

ptomato commented May 14, 2020

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
Copy link
Collaborator

sffc commented May 14, 2020

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

@ptomato
Copy link
Collaborator Author

ptomato commented May 15, 2020

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
Copy link
Member

ljharb commented May 15, 2020

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

@ptomato
Copy link
Collaborator Author

ptomato commented May 15, 2020

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

@sffc
Copy link
Collaborator

sffc commented May 15, 2020

+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
Copy link
Collaborator Author

ptomato commented May 18, 2020

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
Copy link
Collaborator

sffc commented May 18, 2020

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
Copy link
Collaborator Author

ptomato commented May 18, 2020

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?

ptomato added a commit that referenced this issue May 19, 2020
Temporal.MonthDay is cyclical, like Temporal.Time, so it's not
well-defined whether 12-31 should come before or after 01-01. Unlike
Temporal.Time where we resolve the ambiguity by assuming the times occur
in the same day, it's less well-defined for Temporal.MonthDay because
the operands could be in different calendars.

Since Temporal.MonthDay already doesn't have arithmetic, it's not a big
stretch to remove comparison as well.

Closes: #523
ptomato added a commit that referenced this issue May 19, 2020
Temporal.MonthDay is cyclical, like Temporal.Time, so it's not
well-defined whether 12-31 should come before or after 01-01. Unlike
Temporal.Time where we resolve the ambiguity by assuming the times occur
in the same day, it's less well-defined for Temporal.MonthDay because
the operands could be in different calendars.

Since Temporal.MonthDay already doesn't have arithmetic, it's not a big
stretch to remove comparison as well.

Closes: #523
@ryzokuken ryzokuken modified the milestones: Stage 3, Stable API May 20, 2020
ptomato added a commit that referenced this issue May 22, 2020
Temporal.MonthDay is cyclical, like Temporal.Time, so it's not
well-defined whether 12-31 should come before or after 01-01. Unlike
Temporal.Time where we resolve the ambiguity by assuming the times occur
in the same day, it's less well-defined for Temporal.MonthDay because
the operands could be in different calendars.

Since Temporal.MonthDay already doesn't have arithmetic, it's not a big
stretch to remove comparison as well.

Closes: #523
ptomato added a commit that referenced this issue May 25, 2020
Temporal.MonthDay is cyclical, like Temporal.Time, so it's not
well-defined whether 12-31 should come before or after 01-01. Unlike
Temporal.Time where we resolve the ambiguity by assuming the times occur
in the same day, it's less well-defined for Temporal.MonthDay because
the operands could be in different calendars.

Since Temporal.MonthDay already doesn't have arithmetic, it's not a big
stretch to remove comparison as well.

Closes: #523
ptomato added a commit that referenced this issue May 26, 2020
Temporal.MonthDay is cyclical, like Temporal.Time, so it's not
well-defined whether 12-31 should come before or after 01-01. Unlike
Temporal.Time where we resolve the ambiguity by assuming the times occur
in the same day, it's less well-defined for Temporal.MonthDay because
the operands could be in different calendars.

Since Temporal.MonthDay already doesn't have arithmetic, it's not a big
stretch to remove comparison as well.

Closes: #523
ryzokuken pushed a commit that referenced this issue May 26, 2020
Temporal.MonthDay is cyclical, like Temporal.Time, so it's not
well-defined whether 12-31 should come before or after 01-01. Unlike
Temporal.Time where we resolve the ambiguity by assuming the times occur
in the same day, it's less well-defined for Temporal.MonthDay because
the operands could be in different calendars.

Since Temporal.MonthDay already doesn't have arithmetic, it's not a big
stretch to remove comparison as well.

Closes: #523
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

Successfully merging a pull request may close this issue.

6 participants