moment / luxon

⏱ A library for working with dates and times in JS
https://moment.github.io/luxon
MIT License
15.05k stars 730 forks source link

Able to pass invalid localWeekNumber #1590

Open mgarf opened 5 months ago

mgarf commented 5 months ago

Describe the bug When setting the date with Year & Week I would expect not being able to pass an invalid localWeekNumber however when I pass an invalid value. It pulls a date one week into the next year.

To Reproduce

DateTime.local({ zone: 'utc', locale: 'en-US' }).set({ localWeekYear: 2022, localWeekNumber: 1, localWeekday: 1 })
// { ts: 2021-12-26T19:39:12.809Z, zone: UTC, locale: en-US }
DateTime.local({ zone: 'utc', locale: 'en-US' }).set({ localWeekYear: 2021, localWeekNumber: 53, localWeekday: 1 })
// { ts: 2021-12-26T19:39:12.810Z, zone: UTC, locale: en-US }

Actual vs Expected behavior I would expect localWeekNumber to throw an error for passing an invalid week.

Desktop (please complete the following information):

andrewj-bjss commented 1 week ago

Not sure if this is a bug or by design but I suspect the latter.

Looking at the existing tests I can see a test for handling wrapping to the previous year, which is a valid scenario (See note 1), but ideally there should be additional boundary tests that test for week 53 in different locales (which is valid) and weeks beyond this, e.g. Week 54, Week 108, etc.

It looks like weekToGregorian in conversions.js handles this specific case where the localWeekNumber results in a day greater than 365, it just shifts the year accordingly, so my assumption it is by design.

Even if it was decided that it is a bug and the current behaviour needs reviewing then there'd also the possibility (based on responses to other PRs I've seen) that there would be reluctance to merge a PR in case people rely on the current behaviour.

Needs clarification from the repo owners.

note 1 - For clarification:

So if Saturday was January 1st, then: