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

Calendar formatter #660

Closed
wants to merge 1 commit into from
Closed

Calendar formatter #660

wants to merge 1 commit into from

Conversation

ptomato
Copy link
Collaborator

@ptomato ptomato commented Jun 10, 2020

I split this out from #638, for ease of review, because it's able to be merged separately.

Since MonthDay.withYear(year) is not guaranteed to be sufficient to
convert to a Temporal.Date if the calendar uses eras as well as years.
Instead of picking the arbitrary day 1 or year 2004 to convert, use the
refISODay or refISOYear, respectively.
@codecov
Copy link

codecov bot commented Jun 10, 2020

Codecov Report

Merging #660 into main will decrease coverage by 42.79%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##             main     #660       +/-   ##
===========================================
- Coverage   95.99%   53.19%   -42.80%     
===========================================
  Files          17       16        -1     
  Lines        4040     1912     -2128     
  Branches      663      489      -174     
===========================================
- Hits         3878     1017     -2861     
- Misses        159      756      +597     
- Partials        3      139      +136     
Flag Coverage Δ
#test262 53.19% <0.00%> (-0.65%) ⬇️
#tests ?
Impacted Files Coverage Δ
polyfill/lib/intl.mjs 12.50% <0.00%> (-87.50%) ⬇️
polyfill/lib/now.mjs 16.66% <0.00%> (-83.34%) ⬇️
polyfill/lib/timezone.mjs 16.82% <0.00%> (-80.23%) ⬇️
polyfill/lib/date.mjs 46.92% <0.00%> (-46.97%) ⬇️
polyfill/lib/time.mjs 52.55% <0.00%> (-46.79%) ⬇️
polyfill/lib/absolute.mjs 56.66% <0.00%> (-43.34%) ⬇️
polyfill/lib/datetime.mjs 52.38% <0.00%> (-43.30%) ⬇️
polyfill/lib/yearmonth.mjs 58.33% <0.00%> (-39.11%) ⬇️
polyfill/lib/ecmascript.mjs 59.48% <0.00%> (-38.11%) ⬇️
... and 7 more

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 81bbc75...1e1416f. Read the comment docs.

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.

One comment. Apart from that, it LGTM.

Comment on lines 102 to 103
console.log(aa.getEpochMilliseconds(), bb.getEpochMilliseconds());
console.log(formatter.resolvedOptions());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed these

@ptomato ptomato force-pushed the calendar-formatter branch from 31f3d7b to 1e1416f Compare June 10, 2020 17:48
@ptomato
Copy link
Collaborator Author

ptomato commented Jun 10, 2020

Actually, scratch this — both these commits depend on the calendar property, so they can't be separated out after all.

@ptomato ptomato closed this Jun 10, 2020
@ptomato ptomato deleted the calendar-formatter branch June 10, 2020 17:49
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.

2 participants