-
Notifications
You must be signed in to change notification settings - Fork 153
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
Normative: Make ZonedDateTime.toLocaleString work without DateTimeFormat #2522
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2522 +/- ##
==========================================
+ Coverage 95.97% 96.16% +0.18%
==========================================
Files 20 20
Lines 11120 11121 +1
Branches 2124 2137 +13
==========================================
+ Hits 10672 10694 +22
+ Misses 387 366 -21
Partials 61 61
... and 1 file with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
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. Nice fast turnaround on this one!
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.
Looks exactly how I had envisioned
In tc39/proposal-temporal#2522 which reached consensus at the March 2023 TC39 meeting, the functionality of Temporal.ZonedDateTime.p.toLocaleString was changed substantially, to not directly pass the ZonedDateTime to any Intl.DateTimeFormat methods. This adds rewrites of all existing tests for toLocaleString, as well as a few tests to verify that Intl.DateTimeFormat methods no longer support Temporal.ZonedDateTime arguments. As we are rewriting the tests anyway, this also ports all of the Temporal.ZonedDateTime.p.toLocaleString tests that were in staging, to the correct format for the main tree.
In tc39/proposal-temporal#2522 which reached consensus at the March 2023 TC39 meeting, the functionality of Temporal.ZonedDateTime.p.toLocaleString was changed substantially, to not directly pass the ZonedDateTime to any Intl.DateTimeFormat methods. This adds rewrites of all existing tests for toLocaleString, as well as a few tests to verify that Intl.DateTimeFormat methods no longer support Temporal.ZonedDateTime arguments. As we are rewriting the tests anyway, this also ports all of the Temporal.ZonedDateTime.p.toLocaleString tests that were in staging, to the correct format for the main tree.
Test262 tests are in tc39/test262#3814, please have a look at those as well if you have time! |
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.
I'm not sure what changed since the last time I reviewed but the tests look good and I gave an LGTM for the previous iteration :)
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.
Overall looks good, but I commented to discuss whether it should be Temporal or 402 that does some of the processing steps between the ZDT's time zone slot value and the canonical identifier.
In tc39/proposal-temporal#2522 which reached consensus at the March 2023 TC39 meeting, the functionality of Temporal.ZonedDateTime.p.toLocaleString was changed substantially, to not directly pass the ZonedDateTime to any Intl.DateTimeFormat methods. This adds rewrites of all existing tests for toLocaleString, as well as a few tests to verify that Intl.DateTimeFormat methods no longer support Temporal.ZonedDateTime arguments. As we are rewriting the tests anyway, this also ports all of the Temporal.ZonedDateTime.p.toLocaleString tests that were in staging, to the correct format for the main tree.
In tc39/proposal-temporal#2522 which reached consensus at the March 2023 TC39 meeting, the functionality of Temporal.ZonedDateTime.p.toLocaleString was changed substantially, to not directly pass the ZonedDateTime to any Intl.DateTimeFormat methods. This adds rewrites of all existing tests for toLocaleString, as well as a few tests to verify that Intl.DateTimeFormat methods no longer support Temporal.ZonedDateTime arguments. As we are rewriting the tests anyway, this also ports all of the Temporal.ZonedDateTime.p.toLocaleString tests that were in staging, to the correct format for the main tree.
See PR #2479 about which a consensus was not reached. This change allows Temporal.ZonedDateTime.prototype.toLocaleString to work by overriding the time zone at the time of creating an Intl.DateTimeFormat object and formatting the corresponding Temporal.Instant, but disallows calling any of the Intl.DateTimeFormat methods on a Temporal.ZonedDateTime. NOTE: The reference code does not implement the spec exactly as written. It observably modifies the options before passing them to the real Intl.DateTimeFormat constructor. The behaviour described in the spec is the correct behaviour.
Before merging, I should note for completeness that this normative change reached consensus at the March 2023 TC39 meeting. |
See PR #2479 about which a consensus was not reached. This is a fallback solution. This change allows Temporal.ZonedDateTime.prototype.toLocaleString to work by overriding the time zone at the time of creating an Intl.DateTimeFormat object and formatting the corresponding Temporal.Instant, but disallows calling any of the Intl.DateTimeFormat methods on a Temporal.ZonedDateTime.
NOTE: The reference code does not implement the spec exactly as written. It observably modifies the options before passing them to the real Intl.DateTimeFormat constructor. The behaviour described in the spec is the correct behaviour.