Skip to content

Conversation

@ben-allen
Copy link
Contributor

Added tests to Temporal.PlainDate, Temporal.PlainDateTime, and Temporal.ZonedDateTime ensuring weekOfYear only defined for "iso8601" calendar

@ben-allen ben-allen mentioned this pull request Nov 3, 2025
19 tasks
Copy link
Contributor

@ptomato ptomato left a comment

Choose a reason for hiding this comment

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

A couple of things with these tests:

  • They need to test yearOfWeek as well as weekOfYear
  • ISO8601 calendar giving week numbers needs to be tested in the non-Intl-only section (but in fact already is, so you can just drop that part)
  • I think it'd be good to move these into test/intl402/Temporal/{PlainDate,PlainDateTime,ZonedDateTime}/prototype/{weekOfYear,yearOfWeek}/non-iso-week-of-year.js which already tests this but in an incomplete way.

@ben-allen ben-allen force-pushed the weekofyear-only-in-iso-8601 branch from ba3b432 to 634d139 Compare November 18, 2025 21:54
@ben-allen ben-allen marked this pull request as ready for review November 18, 2025 21:56
@ben-allen ben-allen requested a review from a team as a code owner November 18, 2025 21:56
using non-ISO calendar with `PlainDate`, `PlainDateTime`, and
`ZonedDateTime`
@ben-allen ben-allen force-pushed the weekofyear-only-in-iso-8601 branch from 634d139 to e6d760c Compare November 18, 2025 22:42
Copy link
Contributor

@ptomato ptomato left a comment

Choose a reason for hiding this comment

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

LGTM, just remove islamicc from each list - it's an alias so doesn't need to be tested directly.

"islamic-civil",
"islamic-tbla",
"islamic-umalqura",
"islamicc",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"islamicc",

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