-
Notifications
You must be signed in to change notification settings - Fork 158
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
Temporal.TimeZone docs #316
Conversation
Unscientifically adopting Wikipedia's preference of "time zone" above "time zone" and "time-zone".
Codecov Report
@@ Coverage Diff @@
## main #316 +/- ##
==========================================
+ Coverage 78.26% 79.69% +1.42%
==========================================
Files 17 17
Lines 3410 3413 +3
Branches 338 353 +15
==========================================
+ Hits 2669 2720 +51
+ Misses 725 677 -48
Partials 16 16
Continue to review full report at Codecov.
|
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 is really good, but I have a few questions.
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.
Great stuff! Thanks a lot.
## new Temporal.TimeZone(timeZone: string) : Temporal.TimeZone | ||
Since `Temporal.Absolute` and `Temporal.DateTime` do not contain any time zone information, a `Temporal.TimeZone` object is required to convert between the two. | ||
|
||
Finally, the `Temporal.TimeZone` object itself provides access to a list of the time zones in the IANA time zone database. |
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.
@pipobscure is this in the spec, or is it just a helper functionality in the polyfill?
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.
It's a stub in the spec (https://tc39.es/proposal-temporal/#sec-temporal.timezone.prototype-@@iterator) but it does look like it's intentionally there. It was also mentioned elsewhere in the stub documentation before I expanded it.
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.
Does anyone know the motivation for this feature? @pipobscure
Incorrect key name in the return value of ES.BalanceTime was causing durations over 1 day to be cropped with disambiguation = 'balance'.
This used ES.GetTimeZoneOffsetMilliseconds instead of nanoseconds, presumably an old method that no longer exists.
DAYMILLIS was actually an hour. The bug with DST was due to the use of DAYMILLIS in GetTimeZoneEpochValue().
We describe in the spec that time zone names are canonicalized, including capitalization and IANA Link Names. Add tests for constructing a TimeZone from these. As well, test the 'name' property which is supposed to be the canonicalized identifier. Note: Different implementations of Intl.DateTimeFormat canonicalize IANA Link Names differently! This example picks one that happens to canonicalize the same in the IANA and CLDR databases. See for background: https://stackoverflow.com/questions/45867761/javascript-intl-api-gives-different-results-in-chrome
The existing code would get negative hours with nonzero minutes wrong, since it would add positive minutes to negative hours.
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.
Thanks especially for the added coverage and bug fixes. 💯
MDN-style documentation of Temporal.TimeZone, plus minor bug fixes so that the examples work.