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-ignoring compare behaves unexpectedly for non-ISO calendar PlainYearMonth comparisons #1846

Closed
justingrant opened this issue Sep 22, 2021 · 4 comments · Fixed by #1855
Labels
documentation Additions to documentation no-spec-text PR can be ignored by implementors

Comments

@justingrant
Copy link
Collaborator

justingrant commented Sep 22, 2021

At the request of one of the TC39 committee members, we made a change right before Stage 3 to ignore Calendar and TimeZone in compare methods.

The previous behavior was to lexicographically compare calendar and timezone IDs. The new (and current) behavior is to ignore timezone and calendar, so that only ISO fields (and, for ZDT, the instant) is compared.

While working on the docs for this change (#1845) I noticed that this change has very odd results when comparing PlainYearMonth instances across calendars, because comparison currently only compares the ISO dates of the first day of the month. Sometimes, a non-ISO month happens to fall on the first day of an ISO month, which can yield the weird circumstance of two obviously-different months being considered equal by Temporal.

Temporal.PlainYearMonth.compare(
  { year: 2003, month: 1, calendar: 'chinese' },  // first Chinese month of 2003 
  { year: 2003, month: 2, calendar: 'gregory' }  // second Gregorian moth of 2003
) 
// Expected: Not sure, but definitely not zero!
// Actual: 0 

Is this OK? Unlike all other types with a compare method, PlainYearMonth doesn't have a calendar-independent way to decide if two instances are "equal", because months are inherently calendar-specific.

Seems like our choices include:

  1. Leave as-is to be consistent with PlainDate behavior
  2. Change back to use lexicographic comparison
  3. Throw, because month comparisons across calendars make no sense.
  4. Remove the compare method from this type. (EDIT: added this per @ptomato suggestion below)

Are there any other options?

@ptomato
Copy link
Collaborator

ptomato commented Sep 23, 2021

Another option ("option 4") would be to remove PlainYearMonth.compare (see js-temporal/proposal-temporal-v2#18 for why it doesn't exist on PlainMonthDay).

Currently the status quo (option 1) is my preference. This corresponds to the semantics of "comparing the day on which the month starts". Option 4 would correspond to the semantics of "comparing the month as a whole" which is impossible because months that start on the same day can be different lengths, as you pointed out.

I think option 2 is probably a non-starter since we got strong feedback that the lexicographical comparison was not good in compare(). I'd prefer option 4 above option 3 in all respects.

@justingrant
Copy link
Collaborator Author

Good idea re: "remove" option. I added it to the list in the OP. I don't have a strong opinion about which option is best. Let's discuss at next champions meeting?

While thinking about this problem, I added js-temporal/proposal-temporal-v2#21 to add additional "strict" (all fields must match) methods in V2.

@ptomato
Copy link
Collaborator

ptomato commented Sep 30, 2021

Although we were a bit short-handed at the champions meeting today, we discussed this. Shane, Richard, and I all prefer option 1, the status quo. If any champions have a strong preference otherwise, holler soon, and otherwise I'll close this issue.

@justingrant justingrant added documentation Additions to documentation no-spec-text PR can be ignored by implementors labels Oct 1, 2021
@justingrant
Copy link
Collaborator Author

Sounds good to me. Here's an excerpt from meeting notes to explain the decision:

PFC: My preference is the status quo. It makes the semantics of the method 'compare the first day of the month'.
RGN: That's my preference as well. The first day exists on a timeline, so can be compared.
SFC: If you have a list of YearMonths with different calendar systems and you sort them, you end up with a list of months ordered by start date. That's a reasonable result.

Let's leave this issue open as a reminder to update the docs to explain this behavior. Doc text can be something like below.


When comparing Temporal.PlainYearMonth instances in different calendars, the first day of the month is projected into the ISO 8601 calendar, and that date is used to perform comparisons.
For example:

Temporal.PlainYearMonth.compare(
  { year: 2003, month: 1, calendar: 'chinese' },  // first Chinese month of 2003 
  { year: 2003, month: 2, calendar: 'gregory' }  // second Gregorian moth of 2003
) 
// => 0 (because these months start on the same ISO 8601 date: 2003-02-01

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Additions to documentation no-spec-text PR can be ignored by implementors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants