search

tc39 / proposal-canonical-tz

TC39 Proposal (stacked on Temporal) to improve handling of changes to the IANA Time Zone Database
Other
38 stars 2 forks source link

@dminor Stage 3 review #34

Closed justingrant closed 1 year ago

justingrant commented 1 year ago

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 https://github.com/tc39/test262/pull/3837.

Thanks!

dminor commented 1 year ago

Thanks, I'll review this week.

dminor commented 1 year ago

@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 commented 1 year ago

@dminor is is sufficient in the spec.

dminor commented 1 year ago

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

justingrant commented 1 year ago

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 commented 1 year ago

@justingrant The new changes look good to me.

justingrant commented 1 year ago

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 https://github.com/tc39/proposal-temporal/pull/2633. Thanks for the reviews and for your support of this proposal!