-
Notifications
You must be signed in to change notification settings - Fork 461
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: Tests for week-numbering algorithm #4009
Conversation
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! All these tests look good, with one comment about needing to move a few of them.
However I think we need to cover a few more things in order to make sure that engines implement every aspect of this normative change:
- In
test/built-ins/
, validation that a custom calendar may return undefined fromyearOfWeek
andweekOfYear
and that a PlainDate/PlainDateTime/ZonedDateTime with this custom calendar will have theyearOfWeek
/weekOfYear
property with the value of undefined - In
test/intl402/
, a sample case whereiso8601
andgregory
week numbers disagree according to CLDR
I'm happy to merge this after moving those two tests and add the additional coverage in a follow up, or also happy to re-review this with the additional tests added. Let me know what your preference is.
test/built-ins/Temporal/ZonedDateTime/prototype/yearOfWeek/non-iso-week-of-year.js
Outdated
Show resolved
Hide resolved
test/built-ins/Temporal/ZonedDateTime/prototype/weekOfYear/non-iso-week-of-year.js
Outdated
Show resolved
Hide resolved
30a24e9
to
4ba2a79
Compare
I've updated the PR to add the tests you requested. |
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 for the update, all looks good!
4ba2a79
to
c5f991d
Compare
Adds and updates tests for PR: tc39/proposal-temporal#2756