Skip to content

fix: accept day-of-week 7 (Sunday) in the seconds and year cron forms - #263

Merged
potiuk merged 2 commits into
pallets-eco:mainfrom
nikolauspschuetz:fix-dow7-second-year-forms
Nov 1, 2026
Merged

potiuk merged 2 commits into
pallets-eco:mainfrom
nikolauspschuetz:fix-dow7-second-year-forms

Conversation

@nikolauspschuetz

Copy link
Copy Markdown
Contributor

7 is a standard alias for Sunday (0), and croniter accepts it in the classic 5-field form. value_alias skipped the 7 -> 0 conversion in the 6-field (seconds) and 7-field (year) forms, so 7 was rejected there as out of range:

>>> croniter.is_valid("0 0 * * 7")      # 5-field
True
>>> croniter.is_valid("0 0 * * 7 0")    # 6-field
False
>>> croniter.is_valid("0 0 * * 7 0 *")  # 7-field
False

The guard is there to keep rejecting 0 as a day/month in those forms; day-of-week was swept into it. Dropping day-of-week from the two branches makes 7 -> Sunday work in every form while leaving the month/day 0 rejection unchanged. Regression test added; full suite green.

@nikolauspschuetz
nikolauspschuetz force-pushed the fix-dow7-second-year-forms branch from f4e895c to ee823d6 Compare August 16, 2026 16:11
@nikolauspschuetz
nikolauspschuetz marked this pull request as ready for review August 23, 2026 14:23

@potiuk potiuk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM — correct fix at the right layer. Dropping DOW_FIELD from the 6- and 7-field skip lists in value_alias lets 7 alias to Sunday like it does in the 5-field form. After the change, 5-7, 0-7, 7#2, L7, 7/2 and second_at_beginning all behave like 5-field. A side effect is that H(0-7) no longer fails for some hash ids. The new test fails on main, and CI is green.

  • Heads-up: #281 edits the same line, adding DAY_FIELD for the 6-field form (#280). Whichever lands second needs a rebase. The end state should be [DAY_FIELD, MONTH_FIELD] for every form.
  • nit (optional): the test only covers a bare 7. One range (5-7) and one #/L case would also exercise the low, high = value_alias(...) and nth paths.

This review was drafted by an AI-assisted tool and
confirmed by an croniter maintainer. The maintainer
approving this PR has read the findings and signed off. If
something feels off, please reply on the PR and a maintainer
will follow up.

Resolve CHANGELOG.rst conflict with pallets-eco#284 by keeping both entries.

Generated-by: Claude Opus 5
@potiuk
potiuk merged commit 7a4ab37 into pallets-eco:main Nov 1, 2026
potiuk added a commit to abhijeet117/croniter that referenced this pull request Nov 1, 2026
Resolve CHANGELOG.rst conflict with pallets-eco#284 and pallets-eco#263 by keeping all entries.

Generated-by: Claude Opus 5
potiuk added a commit to Harsh23Kashyap/croniter that referenced this pull request Nov 1, 2026
Resolve the value_alias conflict with pallets-eco#263: the 6-field skip list is now [DAY_FIELD, MONTH_FIELD].

Generated-by: Claude Opus 5
potiuk added a commit to TrueFurina/croniter that referenced this pull request Nov 1, 2026
Resolve the value_alias conflict with pallets-eco#263: the 6-field skip list is now [DAY_FIELD, MONTH_FIELD].

Generated-by: Claude Opus 5
potiuk added a commit to hdimer/croniter that referenced this pull request Nov 1, 2026
Resolve conflicts with pallets-eco#284, pallets-eco#263 and pallets-eco#267 by keeping all entries.

Generated-by: Claude Opus 5
potiuk added a commit to Raghuvardhan-Reddy78/croniter that referenced this pull request Nov 1, 2026
Resolve conflicts with pallets-eco#284, pallets-eco#263 and pallets-eco#267 by keeping all entries.

Generated-by: Claude Opus 5
potiuk added a commit to abhijeet117/croniter that referenced this pull request Nov 1, 2026
Resolve conflicts with pallets-eco#284, pallets-eco#263 and pallets-eco#267 by keeping all entries.

Generated-by: Claude Opus 5
potiuk added a commit to Raghuvardhan-Reddy78/croniter that referenced this pull request Nov 1, 2026
Resolve conflicts with pallets-eco#284, pallets-eco#263 and pallets-eco#267 by keeping all entries.

Generated-by: Claude Opus 5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants