Skip to content

Compare and hash fixed-offset timezones by UTC offset - #641

Open
Arborsen wants to merge 2 commits into
qlustered:devfrom
Arborsen:fix/timezone-comparison-466
Open

Arborsen wants to merge 2 commits into
qlustered:devfrom
Arborsen:fix/timezone-comparison-466

Conversation

@Arborsen

@Arborsen Arborsen commented Oct 6, 2026

Copy link
Copy Markdown

Fixes #466.

Changes

  • Compare concrete datetime.timezone objects using their native offset-based equality.
  • Hash fixed-offset timezones by utcoffset(None), so different offsets are distinguishable in sets and unordered comparisons, while equal offsets with different display names remain equal.
  • Add 23 regression/control cases for root/nested values, ordered/unordered lists, sets, UTC aliases, negative and microsecond offsets, and datetime truncation options.

This keeps timezone objects in change reports for in-memory Delta replay. Custom tzinfo and ZoneInfo behavior is outside this change.

Validation

  • Focused datetime/hash tests: 142 passed.
  • Full suite including slow tests: 1,332 passed, 8 skipped; 95% total coverage.
  • Original reproduction and controls: 10 passed.
  • Fatal lint checks and git diff --check: passed.
  • Pyright reports the same 50 errors as unchanged dev; no new diagnostics.
  • Tested with Python 3.12.14.

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, 1 and '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 require safe_to_import={'datetime.timezone'}.

@seperman seperman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.py in 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.timezone instances (deepdiff/diff.py:2190, deepdiff/deephash.py:686). CPython does not allow subclassing datetime.timezone. ZoneInfo, pytz and dateutil zones subclass tzinfo, not timezone. Named and DST zones therefore keep their current code path.
  • datetime objects are dispatched to _diff_datetime/_prep_datetime before 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 uses str(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_hash runs prepare_string_for_hashing(str(result)) at the end of _hash, which adds the same str: prefix a real string gets. So timezone.utc and 'timezone:0:00:00' hash the same, just as 1 and 'int:1' already do. The problem existed before this PR.
  • Hash stability: the DeepHash of any structure that contains a datetime.timezone will 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.t2 might not be a timezone. Then != falls back to identity and values_changed is reported, which seems reasonable.

Tests

  • Reading the code, the new tests in tests/test_diff_datetime.py:15-48 and tests/test_hash.py:99-110 should fail on dev, where all timezones compare and hash as equal. test_timezone_equal_offsets_different_names and test_timezone_hash_ignores_name are controls that also pass on dev.

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 needs default_mapping, and loading a pickled Delta needs safe_to_import={'datetime.timezone'}. A short note would help users.

AI-assisted static review; the code was not executed.

@Arborsen

Copy link
Copy Markdown
Author

Added the changelog entry in Arborsen@d08d746, including the change to hashes containing fixed-offset timezones.
One clarification: DeepDiff([timezone.utc], ['timezone:0:00:00'], ignore_order=True) reports a type change on dev, but returns an empty diff on this PR because the values hash identically. The shared scalar/string namespace already exists, but this specific false negative is introduced here. Would you prefer to address it in this PR before merging, or separately?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants