-
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
RefISOYear and RefISODay internal slots #627
Conversation
Codecov Report
@@ 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
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.
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.
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.
LGTM with just a few minor comments and Shane's comments addressed.
b6347fd
to
3276ff7
Compare
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. |
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.
3276ff7
to
ea78628
Compare
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). |
I mean, it's still hardcoded, just now with some justification 😄 |
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.