Make class Message unhashable #262
Conversation
The class Message defines a __eq__ method, but, does not define a __hash__ method. This makes it unhashable in Python 3. But, it is hashable in Python 2.
| @@ -758,6 +758,8 @@ class Order(Message): | |||
|
|
|||
| """ | |||
|
|
|||
| __hash__ = object.__hash__ | |||
There was a problem hiding this comment.
I think hash should be consistent with implementation of __eq__, so that equal messages have equal hashes.
|
Hi @tvalentyn I've made the change. PTAL. |
| @@ -1060,6 +1058,10 @@ def __ne__(self, other): | |||
| """ | |||
| return not self.__eq__(other) | |||
|
|
|||
| def __hash__(self): | |||
| """Hash by __tags""" | |||
| return hash(self.__tags) | |||
There was a problem hiding this comment.
Looking closer at this, self.__tags is a dictionary, which is unhashable. Looks like we have two options:
- Keep the class unhashable.
- Hash over contents of
__tagsor it's subset, if we can consider Message objects immutable, which we probably cannot.
Is there a reason to believe Message objects are immutable? If not, I think making them unhashable makes most sense.
There was a problem hiding this comment.
self.__tags is a dict! I missed that, sorry 🤦♂️
I think Message should be mutable. Therefore, it should be unhashable.
This class is hashable in Python 2. Making it unhashable would break compatibility in Python 3. But, that is also an option.
If we make it unhashable, we should add __hash__ = None to the class to keep the behaviour consistent in Python 2 as well.
OR
If we want to keep behaviour consistent in Python 3, while not breaking compatibility, we should add __hash__ = object.__hash__ to the class.
There was a problem hiding this comment.
There are good reasons why in Python 3 __hash__ is not implemented by default, in this light, I think the better way to keep compatibility would be to set __hash__ = None. This may require cascading changes for Py2 users of apitools. But same/similar changes would still be needed in downstream codebase to make relevant code Python3-compatible, so that may encourage users of apitools to fix their code sooner than later.
There was a problem hiding this comment.
I agree! I'll make that change :)
ttanay
changed the title
|
@tvalentyn Made the change 🚀 |
|
Looks good to me. |
|
Looping in @houglum who is a committer in this repo - PTAL, thank you. |
|
Sorry for the delay, didn't see this until late. I'll pull this change into our submodule of apitools in gsutil tomorrow, then run all the integration tests to make sure we weren't relying on this. Please do @ me if I forget :) |
|
Thank you @houglum . No worries :) |
|
Can this be merged? |
The class Message defines a
__eq__method, but, does not define a__hash__method. This makes it unhashable in Python 3. But, it ishashable in Python 2.
Issue: #261