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

Able to pass invalid localWeekNumber #1590

Open
mgarf opened this issue Jan 28, 2024 · 1 comment
Open

Able to pass invalid localWeekNumber #1590

mgarf opened this issue Jan 28, 2024 · 1 comment

Comments

@mgarf
Copy link

mgarf commented Jan 28, 2024

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):

  • OS: [e.g. iOS] : Windows
  • Browser [e.g. Chrome 84, safari 14.0] : Chrome 120.0.6099.225
  • Luxon version [e.g. 1.25.0] : 3.4.4
  • Your timezone [e.g. "America/New_York"] : America/Chicago
@andrewj-bjss
Copy link

andrewj-bjss commented Jun 26, 2024

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:

  • US first day of the week is Sunday
  • ISO Standard first day of the week is Monday
  • US first week of the year is the week that has 1st January in it
  • ISO standard first week of the year is the week that has the first Thursday in it

So if Saturday was January 1st, then:

  • First day of first week in US would be Sunday 26th December
  • First day of first week in ISO Standard would be Monday 3rd January

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

No branches or pull requests

2 participants