Repository navigation
Conversation
Resolve conflicts with pallets-eco#284, pallets-eco#263 and pallets-eco#267 by keeping all entries. Generated-by: Claude Opus 5
potiuk
left a comment
There was a problem hiding this comment.
The approach is right: compare UTC instants, and move the inclusive bounds through UTC instead of wall-clock time. I checked it against an oracle built from get_next filtered by UTC instant. That covered Sydney/London fall-back and New York spring-forward under zoneinfo, pytz and dateutil, both directions and both exclude_ends. This logic got 0 wrong out of 14,904 cases, where main got 1,229 wrong. All three new tests fail on main for their stated reasons. One change is needed before merge:
datetime.datebounds regress (_instant,src/croniter/croniter.py:170):croniter_range(date(2020,1,1), date(2020,1,3), "0 0 * * *")yields three datetimes on main and now raisesAttributeError: 'datetime.date' object has no attribute 'utcoffset'. A guard (isinstance(d, datetime)before touchingutcoffset) plus a test would keep it working.- nit:
_instantcallsastimezone(UTC)unguarded, so an aware bound atdatetime.min/datetime.maxwith a non-zero offset raisesOverflowErrorwhere main didn't. - The branch conflicts with
mainagain inCHANGELOG.rst; please mergemainwhen you push the fix.
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.
croniter_range decided both its iteration direction and its stop condition
with `start < stop` and `v < stop`. CPython specifies that comparing two aware
datetimes that share a tzinfo ignores that tzinfo and compares the base
datetimes, and fold is part of neither, so across a DST transition those
comparisons use wall-clock order instead of real elapsed time.
The one-microsecond nudge that makes the bounds inclusive had the same defect
from the other side: arithmetic on an aware datetime moves the wall clock and
resets fold, so a one-microsecond step could move a bound by a whole DST
offset.
Three observable symptoms, one cause:
- Australia/Sydney, a four-minute interval inside the fall-back hour:
croniter_range returned 1 of the 6 fire times it contains and ended
silently, with no exception.
- Europe/London, a reverse range across spring-forward: a fire time 15
minutes below the requested lower bound.
- A forward interval whose bounds read backwards on the wall clock ran the
range backwards.
Stepping the underlying croniter over the same windows gives the right answer
in every case, so croniter_range was answering a question its own iterator
answers correctly.
Bounds and fire times are now compared as instants, and the inclusive-ends
nudge moves by real elapsed time. Naive datetimes carry no offset and take
the unchanged path.
Fixes pallets-eco#259
1977d45 to
9815b10
Compare
|
I rebased this branch onto current 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: My earlier review comments still stand, so I'm now waiting for your response on those. Thanks! |
Fixes #259.
croniter_rangedecides both its iteration direction and its stop conditionwith
start < stopandv < stop. CPython specifies that comparing two awaredatetimes that share a
tzinfoignores thattzinfoand compares the basedatetimes, and
foldis part of neither, so across a DST transition thosecomparisons use wall-clock order rather than real elapsed time.
The one-microsecond nudge that makes the bounds inclusive has the same defect
from the other side: arithmetic on an aware datetime moves the wall clock and
resets
fold, so a one-microsecond step can move a bound by a whole DSToffset. Fixing only the comparison is not enough — the nudge puts the bound
back on the wrong side.
Three symptoms, one cause
Silently returns too few results.
Australia/Sydney, a four-minuteinterval inside the fall-back hour, contains six fire times:
The generator ends early. No exception, no warning — the caller gets a prefix
of the correct answer.
Returns results outside the requested interval.
Europe/London, a reverserange across spring-forward, yields a fire time at UTC 01:00 for a lower bound
of UTC 01:15.
Runs in the wrong direction. A forward interval whose bounds happen to read
backwards on the wall clock (
02:39+11:00→02:26+10:00, which is forwards inreal time) iterates backwards and yields values below the requested start.
In all three cases, stepping the underlying
croniterover the same windowgives the right answer, so
croniter_rangewas answering a question its owniterator already answers correctly.
The change
Two small module-level helpers,
_instantand_shift, and three rewiredcomparison sites in
croniter_range._instantnormalises an aware datetime toUTC for ordering and returns a naive one unchanged;
_shiftmoves a bound byreal elapsed time instead of wall-clock time. The existing
_is_successordoesthe same UTC-normalised comparison for
get_next/get_prev, so this bringscroniter_rangein line with the rest of the module.I did not fold
continto_is_successor: that helper callsastimezoneunguarded, which for a naive datetime would silently assume the machine's local
zone, and
croniter_range's naive path is the common one.Verification
test_croniter_range.py, one per symptom. Each fails onmainfor its own reason and passes after. They assert on local-timeisoformat()strings, matching the neighbouring DST tests, so the expectedvalues show the wall clock running backwards while the sequence advances.
expressions, both directions, both
exclude_ends): zero differencesagainst
main. The change is inert unless a bound is ambiguous or the boundscarry different offsets.
including the 30-minute (
Australia/Lord_Howe) and 45-minute(
Pacific/Chatham) shifts, asserting every yielded instant falls inside therequested bounds and the sequence is monotonic in real time:
mainviolatesthose invariants ~11,000 times, this branch zero times.
pytzis unaffected either way, and I verified that:pytznever shares atzinfoinstance across a transition, so its comparisons were alreadyordering by instant. The
foldhalf of this iszoneinfo-specific.Out of scope
While checking
croniter_rangeagainstget_nextas an oracle I hit adisagreement confined to
Australia/Lord_Howe:get_nextskips a fire timethat
get_prevandmatchboth accept. That is the separate_add_tzinfoissue reported in #258 — it reproduces with a bare
croniter(...)and nocroniter_rangeinvolved, and this branch does not touch it.Used AI assistance on this; I reviewed and tested the change myself.