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

RefISOYear and RefISODay internal slots #627

Merged
merged 2 commits into from
May 29, 2020
Merged

RefISOYear and RefISODay internal slots #627

merged 2 commits into from
May 29, 2020

Conversation

ptomato
Copy link
Collaborator

@ptomato ptomato commented May 27, 2020

Temporal.YearMonth gets a RefISODay internal slot and Temporal.MonthDay
gets a RefISOYear internal slot, as discussed in #391. This will be
required for calendar support.

@codecov
Copy link

codecov bot commented May 27, 2020

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #627   +/-   ##
=======================================
  Coverage   97.82%   97.83%           
=======================================
  Files          16       16           
  Lines        3727     3744   +17     
  Branches      637      638    +1     
=======================================
+ Hits         3646     3663   +17     
  Misses         79       79           
  Partials        2        2           
Flag Coverage Δ
#test262 54.77% <89.28%> (+0.26%) ⬆️
#tests 92.83% <100.00%> (+0.03%) ⬆️
Impacted Files Coverage Δ
polyfill/lib/ecmascript.mjs 97.78% <100.00%> (+<0.01%) ⬆️
polyfill/lib/monthday.mjs 99.11% <100.00%> (+0.04%) ⬆️
polyfill/lib/slots.mjs 100.00% <100.00%> (ø)
polyfill/lib/yearmonth.mjs 97.39% <100.00%> (+0.08%) ⬆️

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 4c260c2...ea78628. Read the comment docs.

@ptomato ptomato added this to the Stable API milestone May 27, 2020
Copy link
Collaborator

@sffc sffc left a comment

Choose a reason for hiding this comment

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

Thanks! I think we'll also need to add the refYear/refDay to getISOFields() (#624), or else custom calendars won't be able to access that data.

docs/monthday.md Outdated 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.

LGTM with just a few minor comments and Shane's comments addressed.

polyfill/lib/ecmascript.mjs Show resolved Hide resolved
polyfill/lib/ecmascript.mjs Show resolved Hide resolved
polyfill/lib/yearmonth.mjs Show resolved Hide resolved
@ptomato ptomato force-pushed the 391-ref-iso-fields branch 2 times, most recently from b6347fd to 3276ff7 Compare May 28, 2020 22:35
@ptomato
Copy link
Collaborator Author

ptomato commented May 28, 2020

Thanks for the reviews, I think all the comments are addressed. Good question about the maximum range, but I currently think the way the PR addressed it is the preferable way. I'll leave this sit for a bit while I work on other things, and come back and merge it later if no objection.

docs/monthday.md Outdated Show resolved Hide resolved
ptomato added 2 commits May 29, 2020 09:14
Temporal.YearMonth gets a RefISODay internal slot and Temporal.MonthDay
gets a RefISOYear internal slot, as discussed in #391. This will be
required for calendar support.
For consistency with the changes in the previous commit.
@ptomato ptomato force-pushed the 391-ref-iso-fields branch from 3276ff7 to ea78628 Compare May 29, 2020 16:22
@sffc
Copy link
Collaborator

sffc commented May 29, 2020

The only real requirement is that the year be a leap year. 1972 is as good as any other leap year. I think it's fine to pick one and make it hard coded. I had also suggested 2000 as a nice round number that is a leap year (not all years mod 100 are leap years).

@ptomato
Copy link
Collaborator Author

ptomato commented May 29, 2020

I mean, it's still hardcoded, just now with some justification 😄

@ptomato ptomato merged commit fbb085c into main May 29, 2020
@ptomato ptomato deleted the 391-ref-iso-fields branch May 29, 2020 19:43
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.

4 participants