Make Timezone and FixedTimezone hashable (#1008) - #1019
Closed
mishra-prince wants to merge 2 commits into
Closed
mishra-prince wants to merge 2 commits into
mishra-prince wants to merge 2 commits into
Conversation
Both classes define __eq__ but not __hash__, which makes Python set __hash__ to None and the instances unhashable (a regression since 3.1.0 when __eq__ was added). Restore hashability by defining __hash__ consistently with __eq__ on both classes. Fixes python-pendulum#1008
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Adjust hashing to remain consistent with equality across Timezone subclasses.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
Restores hashability for Timezone and FixedTimezone, with regression tests for hashing and set membership.
Changes:
- Adds
__hash__implementations to both timezone classes. - Adds hashability and set behavior tests.
| File | Reviewed changes |
|---|---|
tests/tz/test_timezone.py |
Adds hashability and set-membership coverage. |
src/pendulum/tz/timezone.py |
Adds hashing methods; subclass equality can violate the equality/hash invariant. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+72
to
+73
| def __hash__(self) -> int: | ||
| return hash((self.__class__, self.key)) |
__eq__ matches any Timezone/FixedTimezone with the same key/offset, including subclasses, so the hash must not depend on the concrete class. Hash the key (Timezone) and offset (FixedTimezone) directly.
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.
Fixes #1008.
TimezoneandFixedTimezonebecame unhashable in 3.1.0:They were hashable in 3.0.0. The regression comes from commit 6383c07, which added
__eq__to these classes without a matching__hash__. Per the Python data model, defining__eq__without__hash__sets__hash__toNone, making instances unhashable — so they can no longer be used as dict keys or set members.Change
Define
__hash__on bothTimezoneandFixedTimezone, consistent with each class's__eq__:Timezonecompares bykey, so it hashes on(class, key).FixedTimezonecompares by_offset, so it hashes on(class, _offset).This keeps the invariant that equal objects hash equally.
Tests
Added
test_hashabletotests/tz/test_timezone.py, covering both classes: equal timezones hash equally and can be used in sets. Fulltests/tzandtests/datetime/test_timezone.pypass locally (641 passed, 3 skipped). No new ruff errors introduced in the changed files.