msrest-for-python icon indicating copy to clipboard operation
msrest-for-python copied to clipboard

Add __hash__ in Model

Open lmazuel opened this issue 7 years ago • 6 comments

Currently defined in Ansible:

# Quick 'n dirty hash impl suitable for monkeypatching onto SDK model objects (so we can use set comparisons)
def gethash(self):
    if not getattr(self, '_cachedhash', None):
        spec = inspect.getargspec(self.__init__)
        valuetuple = tuple([getattr(self, x, None) for x in spec.args if x != 'self'])
        self._cachedhash = hash(valuetuple)
    return self._cachedhash

This use the __init__ signature, but we can use attribute map instead and don't use inspect. Or even use as_dict:

def __hash__(self):
    if not getattr(self, '_cachedhash', None):
        self._cachedhash = hash(self.as_dict())
    return self._cachedhash

Need to check that all possible internal types are ok (duration, datetime, etc.) but should be fine

FYI @nitzmahone @johanste

Related to https://github.com/ansible/ansible/pull/33624#issuecomment-352855396

lmazuel avatar Dec 19 '17 22:12 lmazuel

Should the hash take read-only/server generated values into account?

johanste avatar Dec 19 '17 22:12 johanste

Currently Ansible is happy without (as you noticed, good catch :)). I have no strong opinion, I think __hash__ does not require unicity (contrary to __eq__, we can have hash(a) == hash(b) and a != b). So this is not a requirement for unicity at least. Is it the good thing? Right now I don't know.

lmazuel avatar Dec 19 '17 23:12 lmazuel

@brettcannon Opinion on this one?

lmazuel avatar Dec 19 '17 23:12 lmazuel

Are Models considered immutable? If they aren't then you should not be able to hash them. After that all the details of what you need to do for __hash__ is covered by the docs at https://docs.python.org/3/reference/datamodel.html#object.hash (e.g. if a == b then hash(a) == hash(b)).

brettcannon avatar Dec 19 '17 23:12 brettcannon

@brettcannon It was a note more than deep thought, sorry if I tagged you too ealier before even do some research... So, now I've read your link and the doc :stuck_out_tongue_winking_eye:

No they aren't immutable. But in most scenario, result from Azure are not changed, so this is interesting. Random ideas:

  • How to raise an exception if we used hash on the obj, and try to change something inside after that? -
  • Add a frozen method to send a frozen read-only model? @nitzmahone @johanste

lmazuel avatar Dec 19 '17 23:12 lmazuel

You could also implement something like __getstate__ and return a tuple of immutable values (which you could then substantiate from and also gives you the ability to pickle the class if you want that functionality).

brettcannon avatar Dec 20 '17 00:12 brettcannon