Repository navigation
fix: make pebble.Notice hashable - #2873
tonyandrewmeyer wants to merge 1 commit into
Conversation
Notice is a frozen dataclass, but its last_data dict field made it unhashable. The field is now left out of the hash, but is still part of equality. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| """The number of times one of these notices has occurred.""" | ||
|
|
||
| last_data: dict[str, str] = dataclasses.field(default_factory=dict[str, str]) | ||
| last_data: dict[str, str] = dataclasses.field(default_factory=dict[str, str], hash=False) |
There was a problem hiding this comment.
Maybe this is okay, but it seems to me that these values should form part of the hash. For example, we could include tuple(sorted(notice.last_data.items())) in the hash (though not sure how to do that with dataclasses, off the top of my head.
There was a problem hiding this comment.
The issue with this is that the hash could change over the course of the object's lifetime, which breaks lookups.
class D:
_hash: int = 1
def __hash__(self):
return self._hash
d1 = D()
d = {d1: 'd1'}
d[d1] # 'd1'
d1._hash = 100
d[d1] # KeyError!There was a problem hiding this comment.
However, the reason we're making it hashable is that we expect last_data to be treated as immutable, even though frozen dataclass guarantees don't extend to mutable containers today ...
There was a problem hiding this comment.
Maybe we'd actually want a KeyError if a user accidentally mutated the object? So we'd be intentionally implementing a __hash__ that can break, so that it breaks loudly on user errors.
There was a problem hiding this comment.
Yeah, last_data is not intended to be changed (I assume) but since we don't have frozendict yet, it's a bit messy.
I think it shouldn't be in the hash if you see a notice as being a Pebble notice, where last_data is basically a shortcut to some extra information, but comparing two notices means comparing the key (but then the dates and occurrences probably shouldn't be in the hash either). It probably should be in the hash if you see a notice as being a snapshot of a Pebble notice at a specific time (more the Python object than the Pebble concept).
I don't think this is a real issue. It came up when I did one of my semi-regular sweeps for issues by an agent, and it seemed worth bundling into a set of small fixes. But it does seem extremely unlikely that anyone does modify last_data in practice. Maybe people want to be able to hash a notice (collecting them in a dict where the key is the dict key?) but no-one is doing that now, since it wouldn't work.
Maybe we should just reject this?
There was a problem hiding this comment.
I'd be in favour of just rejecting this. I agree it seems very unlikely for it to be an issue.
Noticeis frozen but was unhashable because of its dict field. The field is now left out of the hash, but is still part of equality.