Azure / msrest-for-python

The runtime library "msrest" for AutoRest generated Python clients.
MIT License
41 stars 64 forks source link

Add __hash__ in Model #74

Open lmazuel opened 6 years ago

lmazuel commented 6 years ago

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

johanste commented 6 years ago

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

lmazuel commented 6 years ago

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 commented 6 years ago

@brettcannon Opinion on this one?

brettcannon commented 6 years ago

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)).

lmazuel commented 6 years ago

@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:

brettcannon commented 6 years ago

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).