Ensure hashability of classes with custom equality check #363
Conversation
… method The implementation is quite simplistic as it relies uniquely on the memory address location of the object.
…nd Spec instances
Read more about the issue on Yelp/bravado-core#363
| @@ -465,6 +465,9 @@ def __delitem__(self, property_name): | |||
| else: | |||
| del self.__dict[property_name] | |||
|
|
|||
| def __hash__(self): | |||
| return id(self) | |||
There was a problem hiding this comment.
While this seems like a quick fix, I think it violates one of the assumptions of hashes and equality of objects.
If obj1 == obj2 then hash(obj1) == hash(obj2)
What you're doing here is the default for the __hash__ method. Python 3 intentionally removes it since you implemented __eq__. I'm honestly not sure if this is what we want to do, but I also don't have a better idea.
There was a problem hiding this comment.
You're right ... for some reason I was remembering the property in the opposite direction.
I'll be fixing it before merging.
The fix might be based on top of equality check, not great as it will depend on the size of the Swagger Specs but I don't expect this to be a big issue.
If we do this that this is an issue we might just want to remove the "custom" equality checks as they were added mostly for testing purposes
There was a problem hiding this comment.
@sjaensch I'm planning to drop this PR and to create a new PR where the custom __eq__ methods (that made bravado_core.spec.Spec not hashable) are replaced by a different method is_equal.
Technically that change will be backward incompatible, so I'd like to have your thoughts around it.
Ensure hashability of classes with custom equality check.
The issue has been noticed because Yelp/swagger-spec-compatibility tests are broken (due to the fact that
bravado_core.spec.Specinstances are not hashable).In #360 equality methods have been added in order to increase testing coverage of deepcopy of instances.
Objects inheriting from
object(aka all) have:__eq__implemented as always returningFalse__hash__implemented as returning a unique identifier of the specific instance (aka result ofid)Unfortunately, on Python, if you override one
__eq__method then the "default"__hash__implementation is not is use, and so to make the object hashable again you need to implement__hash__yourself (what this CR does). (Documentation reference)