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

Temporal.TimeZone docs #316

Merged
merged 7 commits into from Jan 21, 2020
Merged

Temporal.TimeZone docs #316

merged 7 commits into from Jan 21, 2020

Conversation

ptomato
Copy link
Collaborator

@ptomato ptomato commented Jan 16, 2020

MDN-style documentation of Temporal.TimeZone, plus minor bug fixes so that the examples work.

Unscientifically adopting Wikipedia's preference of "time zone" above
"time zone" and "time-zone".
@codecov
Copy link

codecov bot commented Jan 16, 2020

Codecov Report

Merging #316 into main will increase coverage by 1.42%.
The diff coverage is 100%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
polyfill/lib/regex.mjs 100% <100%> (ø) ⬆️
polyfill/lib/duration.mjs 84.84% <100%> (ø) ⬆️
polyfill/lib/timezone.mjs 82.92% <100%> (+27.98%) ⬆️
polyfill/lib/ecmascript.mjs 92.79% <100%> (+0.32%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7f1e905...8c10128. Read the comment docs.

Copy link
Collaborator

@gibson042 gibson042 left a 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.

docs/timezone.md Outdated Show resolved Hide resolved
docs/timezone.md Show resolved Hide resolved
docs/timezone.md Show resolved Hide resolved
Copy link
Member

@ryzokuken ryzokuken left a 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.
Copy link
Member

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?

Copy link
Collaborator Author

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.

Copy link
Member

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

docs/timezone.md Outdated Show resolved Hide resolved
docs/timezone.md Show resolved Hide resolved
docs/timezone.md Show resolved Hide resolved
polyfill/lib/duration.mjs Show resolved Hide resolved
polyfill/lib/ecmascript.mjs Show resolved Hide resolved
polyfill/lib/timezone.mjs Show resolved Hide resolved
polyfill/lib/timezone.mjs Show resolved Hide resolved
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.
Copy link
Member

@ryzokuken ryzokuken left a 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. 💯

@ryzokuken ryzokuken merged commit c0dbd08 into tc39:main Jan 21, 2020
@ptomato ptomato deleted the timezone-docs branch January 21, 2020 19:02
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

Successfully merging this pull request may close these issues.

None yet

4 participants