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

DifferenceTemporalZonedDateTime: Move time zone equality check next to calendar check? #2533

Closed
anba opened this issue Mar 24, 2023 · 3 comments · Fixed by #2770
Closed

DifferenceTemporalZonedDateTime: Move time zone equality check next to calendar check? #2533

anba opened this issue Mar 24, 2023 · 3 comments · Fixed by #2770
Assignees
Labels
editorial spec-text Specification text involved
Milestone

Comments

@anba
Copy link
Contributor

anba commented Mar 24, 2023

Step 3:

  1. If ? CalendarEquals(zonedDateTime.[[Calendar]], other.[[Calendar]]) is false, then
    a. Throw a RangeError exception.

and step 9:

  1. If ? TimeZoneEquals(zonedDateTime.[[TimeZone]], other.[[TimeZone]]) is false, then
    a. Throw a RangeError exception.

should probably be moved next to each other.

@gibson042
Copy link
Collaborator

This might not be important to change, but having an early return in between two checks that don't affect is undesirable. Will revisit after #2519.

@gibson042 gibson042 added spec-text Specification text involved normative Would be a normative change to the proposal labels Mar 30, 2023
@gibson042 gibson042 added this to the Stage "3.5" milestone Mar 30, 2023
@ptomato
Copy link
Collaborator

ptomato commented Apr 27, 2023

I wrote a comment just now in favour of changing this, and then deleted it, because I realized something that I completely missed on the first read-through.

The time zone check is intentionally done after processing the options argument, because the options determine whether you take the exact-time difference (for which the time zone isn't relevant) or the wall-time difference (for which the time zones must be equal). So I now think we should not make a change here.

@gibson042
Copy link
Collaborator

Ah, right. The polyfill explains it like «When calculating difference between time zones, largestUnit must be 'hours' or smaller because day lengths can vary between time zones due to DST or time zone offset changes.». That should probably be explained in the algorithm with a note.

@ptomato ptomato added editorial and removed normative Would be a normative change to the proposal labels Apr 27, 2023
@ptomato ptomato modified the milestones: Stage "3.5", Stage 4 Apr 27, 2023
@ptomato ptomato self-assigned this Feb 13, 2024
ptomato added a commit that referenced this issue Feb 13, 2024
…e zones

This note explains why we check the time zone equality here, and why we
do it only after reading _options_.

Closes: #2533
ptomato added a commit that referenced this issue Feb 13, 2024
…e zones

This note explains why we check the time zone equality here, and why we
do it only after reading _options_.

Closes: #2533
ptomato added a commit that referenced this issue Feb 14, 2024
…e zones

This note explains why we check the time zone equality here, and why we
do it only after reading _options_.

Closes: #2533
ptomato added a commit that referenced this issue Feb 14, 2024
…e zones

This note explains why we check the time zone equality here, and why we
do it only after reading _options_.

Closes: #2533
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editorial spec-text Specification text involved
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants