Code review comment for lp:~james-w/linaro-image-tools/add-other-relations-to-packages-file

Revision history for this message
Zygmunt Krynicki (zyga) wrote :

You seem to have changed FetchedPackage.__eq__ but did not change __hash__ (it still refers to the old set of fields).

In addition you should not define __eq__ but __cmp__ unless you are prepared to define __ne__. Moreover you should not define __hash__ as your instances are not immutable.

Currently you can create two identical FetchedPackage objects A and B and the following will hold:
assert A == B # because we have __eq__ that works
assert A != B # because __ne__ compares id() by default

The same issue affects TarfileWrongValueMismatch and TarfileMissingPathMismatch

From pydoc SPECIALMETHODS

 There are no implied relationships among the comparison operators.
   The truth of ``x==y`` does not imply that ``x!=y`` is false.
   Accordingly, when defining ``__eq__()``, one should also define
   ``__ne__()`` so that the operators will behave as expected. See
   the paragraph on ``__hash__()`` for some important notes on
   creating *hashable* objects which support custom comparison
   operations and are usable as dictionary keys.

   If a class does not define a ``__cmp__()`` or ``__eq__()`` method
   it should not define a ``__hash__()`` operation either; if it
   defines ``__cmp__()`` or ``__eq__()`` but not ``__hash__()``, its
   instances will not be usable in hashed collections. If a class
   defines mutable objects and implements a ``__cmp__()`` or
   ``__eq__()`` method, it should not implement ``__hash__()``, since
   hashable collection implementations require that a object's hash
   value is immutable (if the object's hash value changes, it will be
   in the wrong hash bucket).

review: Needs Fixing

« Back to merge proposal