Skip to content
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

Make class Message unhashable #262

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions apitools/base/protorpclite/messages.py
Original file line number Diff line number Diff line change
Expand Up @@ -1058,6 +1058,10 @@ def __ne__(self, other):
"""
return not self.__eq__(other)

def __hash__(self):
"""Hash by __tags"""
return hash(self.__tags)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 __tags or 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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@ttanay ttanay Mar 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree! I'll make that change :)



class FieldList(list):
"""List implementation that validates field values.
Expand Down