__eq__ and __ne__ are implemented incorrectly for all classes

[remcohaszing]

__eq__ and __ne__ are implemented incorrectly throughout the entire google-cloud-* code base. I encountered this issue when trying to compare a google.cloud.datastore.Key instance to unittest.mock.ANY.

When implementing custom equality in Python (2 or 3), it should be written like this:

class A:
    def __eq__(self, other):
        if not isinstance(other, A):
            # Delegate comparison to the other instance's __eq__.
            return NotImplemented
        # Whatever logic applies to equality of instances of A can be added here.
        ...

    def __ne__(self, other):
        # By using the == operator, the returned NotImplemented is handled correctly.
        return not self == other

Throughout the entire code base of this repository, equality is implemented like this:

class A:
    def __eq__(self, other):
        if not isinstance(other, A):
            # Other instances are never equal.
            return False
        # Whatever logic applies to equality of instances of A can be added here.
        ...

    def __ne__(self, other):
        # By negating the return value, NotImplemented is treated as False.
        return not self.__eq__(other)

As a result gcloud instances can never equal entities of other classes if they are the first object in the comparison. This is not an issue for most cases, but there are cases where this is an issue. Also this behaviour is simply incorrect.

The __ne__ implementation works, because __eq__ is implemented incorrectly. The boolean value of NotImplemented is True.

>>> not NotImplemented
False

This is typically an issue when unittesting.

>>> from unittest.mock import ANY
>>> 
>>> from google.cloud.datastore import Client
>>> 
>>> 
>>> client = Client()
>>> key = client.key('foo')
>>> key == ANY  # Expected True
False
>>> ANY == key
True
>>> key != ANY  # Expected False
True
>>> ANY != key
False
>>> 

[dhermes]

Thanks @remcohaszing for taking the time to report this! As of 5e86158 it seems to impact 5 subpackages:

$ git grep -l 'def __eq__' | cut -d '/' -f 1 | uniq
bigquery
bigtable
datastore
monitoring
spanner

Unfortunately, it's spread out in an ad-hoc way so fixing everything will take awhile:

$ git grep 'def __eq__' | wc
     36     144    2602
$ git grep 'def __ne__' | wc
     16      64    1159

[lukesneeringer]

@remcohaszing Sanity check: Should the if isinstance(other, A): actually be if not isinstance(other, A):?

[dhermes]

@lukesneeringer I've started to "do this right", e.g. https://github.com/GoogleCloudPlatform/google-cloud-python/blob/26214d229860c5ec128d0d767f1d302f588d49ec/bigquery/google/cloud/bigquery/schema.py#L103-L113

[remcohaszing]

@lukesneeringer You're right, my mistake.

[lukesneeringer]

@dhermes Should I hold off on this then? I was going to knock them all out.

[dhermes]

No, go for it. I was just giving you an example of the "correct" thing.

Any time I've "done the right thing" was for new code.

[lukesneeringer]

Okay, that was what I thought, but I wanted to be polite.