Reject ISO week 53 in years that only have 52 weeks - #1338
Open
Nishuuzz wants to merge 1 commit into
Open
Conversation
`iso_to_gregorian` checks that the week is between 1 and 53, but not every
year has a 53rd ISO week. When it doesn't, the date arithmetic quietly
lands in the next ISO year instead of failing, so `arrow.get()` returns a
date from a year the caller didn't ask for:
>>> arrow.get((2025, 53, 1)).date()
datetime.date(2025, 12, 29)
>>> _.isocalendar()
datetime.IsoCalendarDate(year=2026, week=1, weekday=1)
Both `datetime.date.fromisocalendar(2025, 53, 1)` and arrow's own string
path, `arrow.get("2025-W53-1", "W")`, raise ValueError for the same input.
Check that the computed date really falls in the requested ISO year and
raise if it doesn't. Years that genuinely have 53 weeks are unaffected,
which the existing (2004, 53, 6) and (2009, 53, 6) cases already cover.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1338 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 10 10
Lines 2315 2317 +2
Branches 358 359 +1
=========================================
+ Hits 2315 2317 +2 ☔ View full report in Codecov by Harness. |
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
arrow.get()accepts an ISO calendar tuple, and if you hand it a week the year doesn't have, it doesn't complain — it just hands back a date from the following year:2025 only has 52 ISO weeks. You asked for week 53 of 2025 and got week 1 of 2026, with nothing to tell you that happened. Same for
(2024, 53, 1), which comes back as week 1 of 2025.What makes this look unintended rather than deliberate is that arrow already rejects it everywhere else. Its own string path raises:
and so does the standard library:
so the tuple path is the odd one out.
The cause is in
iso_to_gregorian. It validates that the week is in 1–53 and the day in 1–7, but 53 is only valid for some years — 36 of the 201 years between 1900 and 2100. For the rest, adding 52 weeks to the start of the ISO year simply walks into the next one, and there's no check on the result.The fix
After computing the date, check it actually falls in the ISO year that was asked for, and raise if it doesn't. That's the same shape as the day-of-year rollover guard in #1329, which compares the resulting year against the requested one.
Years that really do have 53 weeks are untouched. The existing
test_one_arg_iso_calendarcases(2004, 53, 6)and(2009, 53, 6)both use week 53 in years that have one, and they still pass.Tests
test_iso_gregorian_week_53covers both sides: week 53 of 2026 resolves normally, week 53 of 2025 and of 2024 raise. I also added the tuple case totest_one_arg_iso_calendar, since that's the public API people actually hit this through.Both fail before the change and pass after it. Full suite is green at 1902 passed, black and flake8 clean.
To convince myself the guard was exact and not just right on the cases I'd picked, I compared
iso_to_gregorianagainstdatetime.date.fromisocalendarfor every (year, week, day) from 1900 to 2100 — 74,571 combinations, counting which ones raise. They now agree on all of them, where before they disagreed on every week 53 of a 52-week year.One thing worth calling out
This is a behaviour change, not just an internal tidy-up.
arrow.get((2025, 53, 1))used to return a date and now raisesValueError. Anyone relying on the old rollover will see a new exception. My read is that returning a silently wrong year is worse than raising, and it lines up with both the stdlib and arrow's own string parser — but it is a break, so it's your call whether it wants a note in the changelog.