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

Some Calendar questions #1425

Closed
4 tasks done
bakkot opened this issue Mar 7, 2021 · 7 comments · Fixed by #1580
Closed
4 tasks done

Some Calendar questions #1425

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

Comments

@bakkot
Copy link
Contributor

bakkot commented Mar 7, 2021

  • delegate review: Calendar and TimeZone should be brand-checked, not normal, objects #1294 Is there a reason Calendars are passed around as objects, instead of eagerly getting all the methods and passing those around? That would also help with validation. (Sorry if there's discussion of this somewhere.)

  • In CalendarEquals there are observable ToString calls. It might be worth doing a preemptive strict equality check, so that the common case of comparing a calendar to itself can avoid calling any user code.

  • The documentation says "calendar.fields [...] is called indirectly when using the from() static methods and with() methods". It's called in a lot more places than that, e.g. in toPlainYearMonth (via CalendarFields); is that omission intentional?

  • Why does CompareCalendar exist? I do not tend to think of calendars as being comparable.

@ptomato
Copy link
Collaborator

ptomato commented Mar 7, 2021

I will try to address all the other points later but as for (1) there is an existing discussion starting at #1294 (comment).

tl;dr: Yes, that would be possible, and has some advantages, but leads to existing PlainWhatever and ZonedDateTime objects being broken if Temporal.Calendar.prototype is mutated, and it's my opinion that that the advantages don't weigh strongly enough.

@bakkot
Copy link
Contributor Author

bakkot commented Mar 7, 2021

Sweet, thanks for the pointer.

@ptomato
Copy link
Collaborator

ptomato commented Mar 8, 2021

(2) Sure, that seems harmless.

(3) I don't think it was intentional. We can just change this.

(4) In, for example, Temporal.PlainDateTime.compare(), if the dates are the same but the calendars are different, then the function needs to return -1 or +1 in order to have some sort order, so we picked lexicographical comparison of the calendar ID. Previous discussion on this and rationale in #588 and #625.

@ptomato ptomato added documentation Additions to documentation non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved and removed question labels Mar 8, 2021
@ptomato ptomato added this to the Next milestone Mar 8, 2021
@bakkot
Copy link
Contributor Author

bakkot commented Mar 8, 2021

if the dates are the same but the calendars are different, then the function needs to return -1 or +1 in order to have some sort order

No, it doesn't. Returning 0 is the correct thing to do for two items for which neither comes before the other. That's a useful thing to do; indeed, that's why we made Array.prototype.sort stable. I'll open a new issue for this.

@ptomato
Copy link
Collaborator

ptomato commented Mar 8, 2021

Personally I agreed with that, but do check #588 and #625 before opening a new issue; there was a large amount of opposition to that position and I thought the reasons given were valid.

@ptomato
Copy link
Collaborator

ptomato commented Apr 16, 2021

Of the above points I believe that only (2) and (3) still apply, and (2) is normative, so I'll make sure that gets done before we say that the Stage 3 conditions have been resolved.

@bakkot
Copy link
Contributor Author

bakkot commented Apr 16, 2021

Note that (2) applies equally to TimeZoneEquals.

ptomato added a commit that referenced this issue Apr 16, 2021
In CalendarEquals, TimeZoneEquals, and ConsolidateCalendars, remove the
observable toString() calls from the path where both calendars are the
same object.

See: #1425
ptomato added a commit that referenced this issue Apr 17, 2021
In CalendarEquals, TimeZoneEquals, and ConsolidateCalendars, remove the
observable toString() calls from the path where both calendars are the
same object.

See: #1425
ptomato added a commit that referenced this issue Apr 17, 2021
In CalendarEquals, TimeZoneEquals, and ConsolidateCalendars, remove the
observable toString() calls from the path where both calendars are the
same object.

See: #1425
@ptomato ptomato mentioned this issue Apr 19, 2021
44 tasks
@ptomato ptomato modified the milestones: Stage 3 Conditional, Next May 12, 2021
Ms2ger added a commit that referenced this issue Jun 28, 2021
ptomato pushed a commit that referenced this issue Jun 28, 2021
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.

2 participants