Repository navigation
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 119e349756
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| and type(dt) is _DATETIME | ||
| and type(getattr(dt, 'tzinfo', None)) is zoneinfo.ZoneInfo |
There was a problem hiding this comment.
When dt uses a non-ZoneInfo timezone that retains the same tzinfo object across a DST transition, such as dateutil.tz.gettz, this exact-type gate skips the UTC conversion and subtraction returns the wall-clock difference rather than elapsed time. That contradicts docs/guide/conversions-and-formatting.md:99-100, which promises elapsed-time handling for all timezone-aware datetimes; either narrow that documented guarantee to plain ZoneInfo datetimes or preserve the behavior for the other advertised inputs.
Useful? React with 👍 / 👎.
#56 converted the current time to UTC when `dt.utcoffset()` was known, so two datetimes that share a tzinfo no longer subtract by wall clock. The subtraction that follows asks the tzinfo for the offset, not `dt`, and a datetime subclass that reports its own offset made the two disagree: a TypeError on CPython and a wrong result on PyPy. Ask `now` for the offset instead. `now` is the plain datetime that `datetime.now` built from the same tzinfo, so it reports the offset the subtraction will use. Datetime subclasses and look-alikes such as `arrow.Arrow` are left to their own subtraction. What working code can notice, against 4.1.1: an aware datetime across a daylight saving change gives the elapsed time instead of the wall-clock difference, whatever its tzinfo. A tzinfo that breaks the tzinfo contract (`utcoffset` raising, returning something other than a timedelta, or an offset of a day or more) now raises Python's own error where 4.1.1 printed a wall-clock difference.
wolph
changed the title
This branch has not been deployed
1 participant
Found by an adversarial pass over
python_utils.formatterson develop after #56 and #57, against the released 4.1.1. Every fix started as a failing test. Only errors whose fix cannot break working code are in here. The rest is listed at the bottom for a decision.timesince
#56 converted the current time to UTC when
dt.utcoffset()was known. The subtraction that follows asks the tzinfo for the offset, notdt, so a datetime subclass with its ownutcoffset()made the two disagree:utcoffset(), tzinfo without one, 30 s ago30 seconds agoTypeErroron CPython,2 hours and 29 minutes agoon PyPy30 seconds agoZoneInfo, dateutil,arrow.Arrow)-1 hours ago-1 hours ago1 hour agotimesinceasksnowfor the offset instead ofdt.nowis the plain datetime thatdatetime.nowbuilt from the same tzinfo, so it reports the offset the subtraction will use. Against develop the change is that one expression.Timestamp, pendulumDateTime) and look-alikes such asarrow.Arroware left to their own subtraction, as in 4.1.1.Compatibility
Three hunt rounds ran on an earlier, longer shape of this fix (a
ZoneInfo-only path, since taken back). The sweeps from all three rounds were re-run on this one:localize(identical on 4.1.1), and 72 freezegun rows in the repeated hour (finding 7 below). 4.1.1 also missed 201ZoneInfo, 201 arrow, 478 dateutil and 603 freezegun rows.ZoneInfozones on 3.10, 3.14 and PyPy 3.11: 0 differ from the elapsed time, noTypeError, no segfault.What working code can notice, against 4.1.1:
utcoffsetraisingNotImplementedError, returning anint, or an offset of a day or more) raises that error fromtimesince, where 4.1.1 printed a wall-clock difference because it never asked for the offset. The earlier shape of this branch pinned the 4.1.1 result for such a tzinfo with a test. That test is gone.For a decision
camel_to_underscoreglues an acronym to a followingAs,IsorUssince 4.1.1 (APIAsStringgivesapias_string,URLIsValidgivesurlis_valid). All 612 names in a 20412-name corpus that 4.1.0 converted correctly and 4.1.1 does not have this shape. The plural rule of 1bb0458 reads thesof the two-letter word as a plural.IDAsis bothID+Asand the plural ofIDA, so any change moves released output one way or the other.timesincesays "ago" for a future datetime. Unchanged since before 4.1.1.apply_recursiveskips lists, tuples and non-dict mappings, returns anOrderedDictas adictand never passes**kwargsto the function. Unchanged since before 4.1.1.FakeDatetimedropsfold. On develop since Count elapsed time across DST in timesince #56.formatters.datetimewithout atimezoneattribute raisesAttributeErrorfor an aware input. On develop since Count elapsed time across DST in timesince #56.now()returns a naive datetime gives a value where 4.1.1 raisedTypeError. On develop since Count elapsed time across DST in timesince #56.