-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use hash
to compare equality in AttributeDict
s
#3104
Use hash
to compare equality in AttributeDict
s
#3104
Conversation
hash
to compare equality in AttributeDict
s
2620a18
to
8af3aae
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm 👍🏼. Just had a minor suggestion.
|
||
ret = dict() | ||
for k, v in d.items(): | ||
if isinstance(v, List): | ||
if isinstance(v, (list, tuple)): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yep... we have to handle the case where there are lists inside of tuples with a recursive check. Nice catch 👍🏼
|
||
|
||
def test_tupleization_and_hashing_passing_random(): | ||
for _ in range(1000): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great test 👌🏼
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is super cool!
755521a
to
2f07ac5
Compare
2f07ac5
to
aa3a172
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
excellent use of random! lgtm
@@ -117,7 +118,9 @@ def __hash__(self) -> int: | |||
return hash(tuple(sorted(tupleize_lists_nested(self).items()))) | |||
|
|||
def __eq__(self, other: Any) -> bool: | |||
if isinstance(other, Mapping): | |||
if isinstance(other, AttributeDict): | |||
return hash(self) == hash(other) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🥳
What was wrong?
Two
AttributeDicts
could have the same hash, but not compare as equal.Also found a bug in
tupleize_lists_nested
where it wouldn't find lists if they were inside a tuple.Related to Issue #2572
How was it fixed?
Updated
AttributeDict.__eq__
to compare hashes if one is being compared to another. It still uses the previous method of converting to dicts for comparison if theother
is another type ofMapping
.In
tupleize_lists_nested
, added tuples to things that need to be recursively tupleized, in case they have unhashable objects within them.Todo:
Cute Animal Picture