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

Date comparisons should not include the calendar #1431

Closed
bakkot opened this issue Mar 8, 2021 · 4 comments · Fixed by #1457
Closed

Date comparisons should not include the calendar #1431

bakkot opened this issue Mar 8, 2021 · 4 comments · Fixed by #1457
Assignees
Labels
documentation Additions to documentation non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved tc39-review

Comments

@bakkot
Copy link
Contributor

bakkot commented Mar 8, 2021

Moving discussion from here: I'd like to revisit the conclusion from #588 and #625. It sounds like it was based on the assumption that two unequal objects must not compare as 0, but that's just not true. Indeed, that's the whole reason we made Array.prototype.sort stable; stability is a property which is useful only when you have conceptually unequal things which compare as 0.

Contrary to the discussion there, there is nothing in the definition of a consistent comparison function which requires it, and it's not a strong precedent in other languages: for example, Java's Comparable does not require this property. In fact it violates it for some of its own built-ins, like BigDecimal, for a very similar reason as we should do so here: two things can be represent the same point in the sort order in question but carry extra data which prevents them from being conceptually equal.

And it is important to omit the calendar for comparisons of dates: 2021-03-08 in the Gregorian calendar does not come before 2021-03-08 in the iso8601 calendar and should not be sorted as such.

(Note that I'm only talking about compare here. I'm fine with including calendars in equals.)

@sffc
Copy link
Collaborator

sffc commented Mar 10, 2021

The decisions from #625 were indeed reached under the assumption that .compare() needed to return 0 if and only if .equals() returned true. If this is not the case, I am in favor of @bakkot's suggested behavior.

@Louis-Aime
Copy link

From the developer's point of view, .compare() could be considered working as ==, that is: after evaluating the dates in possibly different calendars, they happen to designate the same day; whereas .equals() would mean a strict equality, like ===.

@ptomato ptomato added this to the Stage 3 Conditional milestone Mar 19, 2021
@ptomato ptomato added documentation Additions to documentation non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved labels Mar 19, 2021
@ryzokuken ryzokuken self-assigned this Mar 20, 2021
@justingrant
Copy link
Collaborator

I'm OK with this change (I don't have a strong opinion either way) but wanted to point out a quirk of this change as related to future extension of comparison methods.

First, some background: in V1, Temporal has compare and equals. There is no method for "less than", "greater than", "less than or equal to", nor "greater than or equals" operations. Nor are Temporal objects comparable using built-in comparison operators <, >, >=, nor <=. In V1, the only way to perform those non-equality operations is to use compare.

However, we've received feedback for more ergonomic support for those comparisons. We're not including this support in V1, but may consider for a future Temporal proposal. See js-temporal/proposal-temporal-v2#6. Also, if comparison operator overloading becomes part of ECMAScript, then presumably we'd want Temporal objects to participate.

If we do end up extending comparison support in the future, then the "less than" and "greater than" operations would presumably take calendar and time zone into account, because otherwise there'd be cases where lessThan, equals, and greaterThan could all return false for the same two objects. In this possible future implementation, if you are comparing two values with the same ISO date or instant but different calendars and/or timezones, then you can end up with a case where a.lessThan(b) === true but PlainDate.compare(a, b) === 0.

Is this a problem? I assume it's NOT a problem (for the same reasons why a.equals(b) can be false even if PlainDate.compare(a, b) === 0 in V1) but at least wanted to highlight this quirk to the folks on this thread in case there are concerns.

ryzokuken added a commit to ryzokuken/proposal-temporal that referenced this issue Mar 22, 2021
This commit changes different compare methods on types that include
calendars to not take them into account. With this, two instances that
have different calendars attached but map to the same points in the ISO
calendar are compared to be "equal".

Fixes: tc39#1431
ryzokuken added a commit to ryzokuken/proposal-temporal that referenced this issue Mar 22, 2021
This commit updates the polyfill to reflect the spec changes made for
addressing tc39#1431.
@bakkot
Copy link
Contributor Author

bakkot commented Mar 22, 2021

@justingrant:

I'd object to lessThan and greaterThan taking into account time zones or calendars for the same reason I objected to it for compare.

otherwise there'd be cases where lessThan, equals, and greaterThan could all return false for the same two objects

There's nothing obviously wrong with that; it just means Temporal instances would be partially ordered rather than totally ordered. (Which I'd argue is exactly appropriate: 2021-03-08 in the Gregorian calendar is neither before nor after 2021-03-08 in the iso8601 calendar, and yet is also not the same thing.)

ryzokuken added a commit to ryzokuken/proposal-temporal that referenced this issue Mar 22, 2021
This commit updates the polyfill to reflect the spec changes made for
addressing tc39#1431.
ryzokuken added a commit to ryzokuken/proposal-temporal that referenced this issue Mar 23, 2021
This commit changes different compare methods on types that include
calendars to not take them into account. With this, two instances that
have different calendars attached but map to the same points in the ISO
calendar are compared to be "equal".

Fixes: tc39#1431
ryzokuken added a commit to ryzokuken/proposal-temporal that referenced this issue Mar 23, 2021
This commit updates the polyfill to reflect the spec changes made for
addressing tc39#1431.
ryzokuken added a commit to ryzokuken/proposal-temporal that referenced this issue Mar 23, 2021
This commit updates the polyfill to reflect the spec changes made for
addressing tc39#1431.
ptomato pushed a commit that referenced this issue Mar 23, 2021
This commit changes different compare methods on types that include
calendars to not take them into account. With this, two instances that
have different calendars attached but map to the same points in the ISO
calendar are compared to be "equal".

Fixes: #1431
ptomato pushed a commit that referenced this issue Mar 23, 2021
This commit updates the polyfill to reflect the spec changes made for
addressing #1431.
ptomato pushed a commit to js-temporal/temporal-polyfill that referenced this issue May 6, 2021
This commit updates the polyfill to reflect the spec changes made for
addressing tc39/proposal-temporal#1431.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Additions to documentation non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved tc39-review
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants