-
Notifications
You must be signed in to change notification settings - Fork 483
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
Update UTC time zone canonicalization to match proposal-canonical-tz #4336
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems fine, but should this test be relocated out of the test/intl402/DateTimeFormat/
folder into a Temporal-specific folder?
Alternatively, maybe this test should both be copied into a Temporal folder, AND have a test remain here that instead of validating that DataTimeFormat canonicalizes zone IDs (current behavior) the test should validate that it's not canonicalized (new behavior)? Note that V8 is not planning to change the canonicalization behavior of DateTimeFormat until Temporal Stage 4, so it may be premature to land a DTF non-canonical change now.
575a974
to
0055321
Compare
I like that suggestion and I've adopted it now.
Things that are stage 3 are generally cleared to land in test262. Especially now that we have a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I suggested some syntax tweaks and assertion strengthening. I also wonder if we should replace const
syntax with var
and for (const timeZone of utcIdentifiers) { … }
syntax with for (var i = 0; i < utcIdentifiers.length; i++) { var timeZone = utcIdentifiers[i]; … }
, but that might be too extreme (though I suspect @ljharb may also be in favor of it).
test/intl402/Temporal/ZonedDateTime/prototype/equals/canonicalize-utc-timezone.js
Outdated
Show resolved
Hide resolved
I’m in favor of it, but our policy is to not put undue burden on PR authors by requesting those changes, and instead to change them in future PRs. |
The test for tc39/ecma402#724 (added in tc39#4328) didn't take the Time Zone Canonicalization proposal into account; but it should, because that proposal is stage 3. As of that proposal, the [[TimeZone]] slot of DateTimeFormat gets the case-regularized original identifier, no longer the primary identifier. So the resolvedOptions().timeZone property also no longer returns the primary identifier.
0055321
to
75e831c
Compare
I've gone ahead and made the ES5 changes as well, it's a tiny PR so why not |
The test for tc39/ecma402#724 (added in #4328) didn't take the Time Zone Canonicalization proposal into account; but it should, because that proposal is stage 3.
As of that proposal, the [[TimeZone]] slot of DateTimeFormat gets the case-regularized original identifier, no longer the primary identifier. So the resolvedOptions().timeZone property also no longer returns the primary identifier.