Repository navigation
Conversation
seperman
left a comment
There was a problem hiding this comment.
Static review: #641 Compare and hash fixed-offset timezones by UTC offset
Summary: This PR adds a datetime.timezone branch to DeepDiff._diff, which compares with != (by offset), and to DeepHash._hash, which hashes as timezone:<utcoffset>. On dev, a bare datetime.timezone goes through the generic object path. After dunder members are filtered out, only the class-level min/max/utc are left to compare, so all fixed-offset timezones compare and hash as equal. Reading the code, this PR fixes #466 for direct, nested, ordered, unordered and set comparisons.
Overlap with other PRs
- No other open or merged PR addresses #466, which is still open.
- #517 (closed, not merged) dealt with datetimes losing their tzinfo in
datetime_normalize. That is a different code path, and this PR does not touch it: datetime hashing and comparison are unchanged. - The PR merges cleanly with #639 and #642, which also change
deephash.pyin different hunks.
Security
No security concerns found in static review. The PR does not change serialization or the allowlist. As the description says, datetime.timezone is still not in SAFE_TO_IMPORT.
Correctness
- The change applies only to actual
datetime.timezoneinstances (deepdiff/diff.py:2190,deepdiff/deephash.py:686). CPython does not allow subclassingdatetime.timezone.ZoneInfo, pytz and dateutil zones subclasstzinfo, nottimezone. Named and DST zones therefore keep their current code path. datetimeobjects are dispatched to_diff_datetime/_prep_datetimebefore the new branch, so their behavior does not change.- Equality ignores the display name. This matches
timezone.__eq__and what #466 expects (b == d). The hash usesstr(utcoffset(None)), which is unique for each offset, including negative and microsecond offsets. - The "known limitation" in the PR description is correct. For non-string results,
apply_hashrunsprepare_string_for_hashing(str(result))at the end of_hash, which adds the samestr:prefix a real string gets. Sotimezone.utcand'timezone:0:00:00'hash the same, just as1and'int:1'already do. The problem existed before this PR. - Hash stability: the DeepHash of any structure that contains a
datetime.timezonewill change. Before, all such zones hashed the same, so this is the intended fix, but a CHANGELOG line would help. - With
ignore_type_in_groups,level.t2might not be atimezone. Then!=falls back to identity andvalues_changedis reported, which seems reasonable.
Tests
- Reading the code, the new tests in
tests/test_diff_datetime.py:15-48andtests/test_hash.py:99-110should fail ondev, where all timezones compare and hash as equal.test_timezone_equal_offsets_different_namesandtest_timezone_hash_ignores_nameare controls that also pass ondev.
Suggestions
- Add a CHANGELOG entry saying that fixed-offset timezones are now compared and hashed by offset, so hashes that include them change.
- Optional: timezones can now appear in reports. Calling
to_json()on such a diff needsdefault_mapping, and loading a pickled Delta needssafe_to_import={'datetime.timezone'}. A short note would help users.
AI-assisted static review; the code was not executed.
|
Added the changelog entry in Arborsen@d08d746, including the change to hashes containing fixed-offset timezones. |
Fixes #466.
Changes
datetime.timezoneobjects using their native offset-based equality.utcoffset(None), so different offsets are distinguishable in sets and unordered comparisons, while equal offsets with different display names remain equal.This keeps timezone objects in change reports for in-memory Delta replay. Custom
tzinfoandZoneInfobehavior is outside this change.Validation
git diff --check: passed.dev; no new diagnostics.Known limitation for review
The timezone hash follows the existing scalar hashing convention, which shares a namespace with user strings. Consequently,
DeepDiff([timezone.utc], ['timezone:0:00:00'], ignore_order=True)returns an empty diff with this change, whereas the baseline reports a type change. Equivalent collisions already exist for other scalars (for example,1and'int:1'). This patch does not redesign hash domain separation; maintainer guidance on the intended scope is welcome.Existing timezone serialization requirements are unchanged: JSON output needs an appropriate
default_mapping, and persisted Delta loading may requiresafe_to_import={'datetime.timezone'}.