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

@dminor Stage 3 review #34

Closed
justingrant opened this issue Jun 20, 2023 · 7 comments
Closed

@dminor Stage 3 review #34

justingrant opened this issue Jun 20, 2023 · 7 comments

Comments

@justingrant
Copy link
Collaborator

justingrant commented Jun 20, 2023

This is a tracking issue for @dminor to review the Time Zone Canonicalization proposal spec at https://tc39.es/proposal-canonical-tz/#sec-canonical-tz-intro. Dan, feel free to add feedback via comments here or open new issues, whichever is easiest.

The spec text is easiest to review as a web page rather than the raw spec text, because most of the text is just existing AOs with only one line changed. The total scope of changes is <15 lines + one <emu-note>.

Tests can be reviewed at tc39/test262#3837.

Thanks!

@dminor
Copy link

dminor commented Jun 26, 2023

Thanks, I'll review this week.

@dminor
Copy link

dminor commented Jun 26, 2023

@justingrant I have a question about https://tc39.es/proposal-canonical-tz/#sec-temporal-timezoneequals, specifically:

If recordOne is not empty and recordTwo is not empty and recordOne.[[PrimaryIdentifier]] is recordTwo.[[PrimaryIdentifier]], return true.

Is it significant that the text says recordOne.[[PrimaryIdentifier]] is recordTwo.[[PrimaryIdentifier]]? I would have expected it to say is equal to.

@ljharb
Copy link
Member

ljharb commented Jun 26, 2023

@dminor is is sufficient in the spec.

@dminor
Copy link

dminor commented Jun 27, 2023

I've reviewed the spec changes and the tests and they look good to me. @justingrant thank you for taking on this work :)

@justingrant
Copy link
Collaborator Author

FYI: Temporal just merged an editorial PR to prepare for tc39/proposal-temporal#2607, which is the normative PR that limits offset time zones to minute precision.

As a result, today a few links in the proposal-canonical-tz spec text were broken, and a few lines of the proposal spec needed updating. I just landed a PR that catches up this proposal's spec and polyfill with those upstream changes. Apologies for the churn!

If you've already reviewed the rest of proposal-canonical-tz, please take a look at SystemTimeZoneIdentifier that has 2 changed lines + a new editor's note, and TimeZoneEquals that has 5 newly-changed lines.

AFAIK there are no more changes expected (either here or upstream) before the Bergen meeting.

@dminor
Copy link

dminor commented Jul 1, 2023

@justingrant The new changes look good to me.

@justingrant
Copy link
Collaborator Author

This proposal reached Stage 3 at the July 2023 TC39 meeting. As decided in the meeting, the next step is to merge this proposal into Temporal Stage 3 via tc39/proposal-temporal#2633. Thanks for the reviews and for your support of this proposal!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants