Skip to content

Do not report a gap-shifted DST alternative as a fire time - #266

Open
abhijeet117 wants to merge 2 commits into
pallets-eco:mainfrom
abhijeet117:fix/dst-gap-shift-fire-time
Open

abhijeet117 wants to merge 2 commits into
pallets-eco:mainfrom
abhijeet117:fix/dst-gap-shift-fire-time

Conversation

@abhijeet117

Copy link
Copy Markdown
Contributor

Summary

For an hourly schedule in Australia/Lord_Howe, the only zone with a 30-minute DST shift, get_next, get_prev and match disagreed about whether the gap-manufactured 02:30 wall clock fires. When a slot fell into the spring-forward gap, the alternative resolution across the offset change walked forward to the next existing wall clock and returned it although its fields did not match the expression. Such an alternative is now discarded when its resolution had to be shifted, mirroring the wildcard-hour guard already applied to the regular candidate.

Testing

Reproduced on master: get_prev from 03:00 returned 02:30+11:00 and match("0 * * * *", 02:30) was true; both now agree with get_next. Added regression tests for zoneinfo and pytz covering all three APIs. Suite passes apart from four pre-existing failures identical on clean master.

Checklist

  • Bug reproduced before the fix
  • Root cause identified
  • Bug fixed
  • Tests passed

abhijeet117 added a commit to abhijeet117/croniter that referenced this pull request Aug 25, 2026
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 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.

The code change is correct and narrowly scoped. I instrumented main to log every case where a gap-shifted alternative is returned, across 7 zones × zoneinfo/pytz × 13 expressions, both directions. Every case was Australia/Lord_Howe with a wildcard hour returning a non-matching 02:30, so the guard removes only bogus results, and fixed-hour expressions are untouched. Changes needed:

  • Tests miss the get_next half of the bug (src/croniter/tests/test_croniter.py:1848, :1868). get_next from 01:43 already returns 03:00 on main, but from 01:03 main returns 02:30+11:00. The same happens for 15 * * * * from 01:17 and */20 * * * * from 01:45. Please add one of those, plus a croniter_range assertion, since the CHANGELOG says croniter_range is fixed.
  • Wording: "that get_next skips" in the description and CHANGELOG isn't accurate; get_next also returned 02:30 for some start times.
  • nit: the pytz test lacks the 0,30 * * * * positive-control match that the zoneinfo test has.
  • The branch conflicts with main again in CHANGELOG.rst; please merge main when you push.

This review was drafted by an AI-assisted tool and
confirmed by an croniter maintainer. After you've
addressed the points above and pushed an update, an croniter
maintainer — a real person — will take the next look
at the PR. The findings cite the project's review criteria;
if you think one of them is mis-applied, please reply on the
PR and a maintainer will weigh in.

When a cron slot fell into a spring-forward gap and the offset changed,
the alternative resolution walked forward to the next existing wall clock
and returned it even when its fields did not match the expression. For
"0 * * * *" in Australia/Lord_Howe (30-minute shift), get_next skipped
from 01:00 to 03:00 while get_prev from 03:00 landed on 02:30 and
match(02:30) was true. Discard the alternative when its resolution had to
be shifted, mirroring the wildcard-hour guard applied to the regular
candidate, so all three APIs agree again.
@potiuk
potiuk force-pushed the fix/dst-gap-shift-fire-time branch from 06068f6 to 79ee77a Compare November 4, 2026 09:18
@potiuk

potiuk commented Nov 4, 2026

Copy link
Copy Markdown
Contributor

I rebased this branch onto current main so it's up to date, and CI is green on the new head (79ee77a). The content of your change is the same.

The conflicts were only between separate additions, and I kept both sides.

Because this was a force-push, please reset your local branch before pushing again:

git fetch <your-fork-remote>
git reset --hard <your-fork-remote>/fix/dst-gap-shift-fire-time

My earlier review comments still stand, so I'm now waiting for your response on those. Thanks!

This branch has not been deployed

No deployments
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