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

Ignore units not in the data type when formatting with Intl #586

Merged
merged 1 commit into from
May 19, 2020

Conversation

ptomato
Copy link
Collaborator

@ptomato ptomato commented May 18, 2020

If the formatting options request a time zone name for any type except
Absolute, or a day for YearMonth, or a year for MonthDay, we want to
ignore that, since the data model for that type doesn't contain those
units.

Closes: #572

If the formatting options request a time zone name for any type except
Absolute, or a day for YearMonth, or a year for MonthDay, we want to
ignore that, since the data model for that type doesn't contain those
units.

Closes: #572
@codecov
Copy link

codecov bot commented May 18, 2020

Codecov Report

Merging #586 into main will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #586   +/-   ##
=======================================
  Coverage   97.73%   97.74%           
=======================================
  Files          16       16           
  Lines        3584     3599   +15     
  Branches      603      605    +2     
=======================================
+ Hits         3503     3518   +15     
  Misses         79       79           
  Partials        2        2           
Flag Coverage Δ
#test262 54.20% <0.00%> (-0.22%) ⬇️
#tests 92.62% <100.00%> (+0.03%) ⬆️
Impacted Files Coverage Δ
polyfill/lib/intl.mjs 100.00% <100.00%> (ø)

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 6a27973...3ffccba. Read the comment docs.

@sffc
Copy link
Collaborator

sffc commented May 19, 2020

Does this need a spec change, too?

@ptomato
Copy link
Collaborator Author

ptomato commented May 19, 2020

Does this need a spec change, too?

I don't think so (https://tc39.es/proposal-temporal/#sec-partitiondatetimepattern), time zone doesn't seem to be requested for any of those types. (Although I'm not so up to date on that part of the spec and it has a big TODO on it anyway.)

@sffc
Copy link
Collaborator

sffc commented May 19, 2020

OK, yeah, it looks like timeZoneName is not in the Supported Fields table. We should add it as supported for Temporal.Absolute. I filed #591 to follow up on that.

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.

Looks great, thanks.

@ryzokuken ryzokuken merged commit e534d27 into main May 19, 2020
@ryzokuken ryzokuken deleted the 572-ignore-formatting-units branch May 19, 2020 09:54
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.

Time zone and DateTime.prototype.toLocaleString
3 participants