Return to Revisions

1. Review

1. It would be substantially simpler to use a mixin instead of a decorator.

2. The test for equality is not symmetric: you have a == b if b belongs to a subclass of a's class, but not vice versa. So it's possible to have a == b but b != a, which makes no sense.

There are two sensible things to do here: either (i) ignore the class relationship, so that two objects compare equal if their vars are equal, regardless of which class they belong to; or (ii) insist that classes match, so that two objects compare equal only if they belong to the same class.

1. The intention seems to be for eqhash objects to hash based on their instance attributes, so that instances with different attributes get different hashes. But the code doesn't work! Here are two objects with different instance attributes, but whose hashes are the same:

    >>> @eqhash
    ... class Data(SData):
    ...     pass
    >>> d = Data(1)
    >>> hash(d)
    -9223372036573986785
    >>> e = Data(2)
    >>> hash(e)
    -9223372036573986785
   

And here is one object that has different hashes at different times:

    >>> d = Data(1)
    >>> hash(d)
    -9223372036573986785
    >>> vars(d).values()
    dict_values([1])
    >>> hash(d)
    -9223372036574014153

The problem is that you are constructing a dict_values object and then taking its hash. But dict_values objects don't hash based on their contents, only on their id (see Python issue 22192), so the hash doesn't tell you anything about the values, only about the location in memory of the dict_values object.

2. Revised code

class EqHash:
    """Mixin adding __eq__, __ne__, and __hash__ methods."""
    def __eq__(self, other):
        return (self is other
                or (type(self) == type(other)
                    and vars(self) == vars(other)))

    def __ne__(self, other):
        return not (self == other)

    def __hash__(self):
        return hash(tuple(sorted(vars(self).items())))

class Repr:
    """Mixin adding a __repr__ method."""
    def __repr__(self):
        return '{name}({values})'.format(
            name=type(self).__name__,
            values=', '.join(map(repr, vars(self).values())))

3. Answers to questions

1. functools.total_ordering is a decorator becuase it looks at the class in order to determine which ordering method was provided. See the implementation, which calls getattr(cls, op, None). It would be much harder to do this in a mixin.

2. I spotted the problems by thinking carefully!

First, equality is a symmetric relation, but isinstance is asymmetric, so it was suspicious that one was implemented using the other.

Second, it only makes sense to implement __hash__ on immutable objects, but the values of a dictionary are mutable, so this was also suspicious. See the documentation for __hash__, where it says:

If a class defines mutable objects and implements an __eq__ method, it should not implement __hash__, since the implementation of hashable collections requires that a key’s hash value is immutable (if the object’s hash value changes, it will be in the wrong hash bucket).