Fix incorrect DateTime ordering during DST fold transitions#985
Open
Uttkarsh700 wants to merge 5 commits into
Open
Fix incorrect DateTime ordering during DST fold transitions#985Uttkarsh700 wants to merge 5 commits into
Uttkarsh700 wants to merge 5 commits into
Conversation
…ons to UTC via astimezone and add regression test for DST fold
There was a problem hiding this comment.
Pull request overview
Fixes incorrect relational ordering of pendulum.DateTime values during DST “fold” transitions by normalizing operands to UTC before performing <, <=, >, and >= comparisons (aligning behavior with the documented “compare in UTC” semantics).
Changes:
- Add rich ordering operator overrides (
__lt__,__le__,__gt__,__ge__) that normalize both operands to UTC prior to comparison. - Add a regression test covering the DST fold ordering scenario.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/pendulum/datetime.py |
Implements UTC-normalized rich ordering operations intended to fix fold-related mis-ordering. |
tests/datetime/test_comparison.py |
Adds a regression test for fold-related < ordering behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+557
to
+577
| # Rich ordering operations | ||
| def __lt__(self, other: datetime.datetime) -> bool: # type: ignore[override] | ||
| if not isinstance(other, datetime.datetime): | ||
| return NotImplemented | ||
|
|
||
| # Normalize both operands to UTC stdlib datetimes using base implementation | ||
| self_utc = datetime.datetime.astimezone(self, UTC) | ||
| other_dt = other if isinstance(other, DateTime) else DateTime.instance(other) | ||
| other_utc = datetime.datetime.astimezone(other_dt, UTC) | ||
|
|
||
| return datetime.datetime.__lt__(self_utc, other_utc) | ||
|
|
||
| def __le__(self, other: datetime.datetime) -> bool: # type: ignore[override] | ||
| if not isinstance(other, datetime.datetime): | ||
| return NotImplemented | ||
|
|
||
| self_utc = datetime.datetime.astimezone(self, UTC) | ||
| other_dt = other if isinstance(other, DateTime) else DateTime.instance(other) | ||
| other_utc = datetime.datetime.astimezone(other_dt, UTC) | ||
|
|
||
| return datetime.datetime.__le__(self_utc, other_utc) |
Comment on lines
+579
to
+597
| def __gt__(self, other: datetime.datetime) -> bool: # type: ignore[override] | ||
| if not isinstance(other, datetime.datetime): | ||
| return NotImplemented | ||
|
|
||
| self_utc = datetime.datetime.astimezone(self, UTC) | ||
| other_dt = other if isinstance(other, DateTime) else DateTime.instance(other) | ||
| other_utc = datetime.datetime.astimezone(other_dt, UTC) | ||
|
|
||
| return datetime.datetime.__gt__(self_utc, other_utc) | ||
|
|
||
| def __ge__(self, other: datetime.datetime) -> bool: # type: ignore[override] | ||
| if not isinstance(other, datetime.datetime): | ||
| return NotImplemented | ||
|
|
||
| self_utc = datetime.datetime.astimezone(self, UTC) | ||
| other_dt = other if isinstance(other, DateTime) else DateTime.instance(other) | ||
| other_utc = datetime.datetime.astimezone(other_dt, UTC) | ||
|
|
||
| return datetime.datetime.__ge__(self_utc, other_utc) |
Comment on lines
+418
to
+419
| assert d1.timestamp() > d2.timestamp() | ||
| assert not (d1 < d2) |
4ba4509 to
4ecded1
Compare
4ecded1 to
6f3a680
Compare
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.
Summary
Fix incorrect ordering comparisons for
DateTimeinstances during DST fold transitions.During the repeated hour after a daylight saving time (DST) rollback, two datetimes can represent different UTC instants while sharing the same timezone object. In this situation, ordering comparisons (
<,<=,>,>=) can return results that do not reflect the actual chronological order.This change normalizes both operands to UTC before performing ordering comparisons, ensuring that comparisons are based on the represented instant rather than the ambiguous local wall-clock time.
Problem
The issue can be reproduced with datetimes created during the DST fall-back transition:
Although
d1represents a later UTC instant thand2, the ordering comparison reports the opposite result.After reproducing the issue locally, I verified that the problem only affects relational ordering operators. Equality behavior remains unchanged.
Solution
The ordering operators now normalize both operands to UTC before performing the comparison.
The implementation:
astimezone(),datetime.datetimeinstances,NotImplementedfor unsupported comparison types,The change is intentionally limited to ordering comparisons and does not modify equality behavior.
Why this approach?
Pendulum's comparison documentation states that comparisons are performed in UTC. Normalizing both operands before ordering comparisons keeps the implementation aligned with that documented behavior while limiting the scope of the change to relational operators only.
Testing
Added a regression test covering the DST fold scenario:
test_less_than_with_foldValidation performed locally:
tests/datetime/test_comparison.py— 37 passedtests/datetime— 565 passedThis change is intentionally scoped to ordering comparisons and does not modify equality semantics.
Fixes #855