vaab / colour

Python color representations manipulation library (RGB, HSL, web, ...)
BSD 2-Clause "Simplified" License
319 stars 41 forks source link

inequality unexpected behavior with color objects #26

Closed skorokithakis closed 7 years ago

skorokithakis commented 8 years ago
In [1]: second_col.web
Out[1]: 'white'

In [2]: second_col == Color("white")
Out[2]: True

In [3]: second_col != Color("white")
Out[3]: True
tommilligan commented 7 years ago

Nice find! Good case for learning about custom comparisons for classes. Root cause was that the Color class has a custom __eq__ method, but no custom __ne__ method.

skorokithakis commented 7 years ago

Ah, right. You might want to define __ne__ as !__eq__, to make those complementary.

skorokithakis commented 7 years ago

(I.e. define one in terms of the other, that way there's no way to get something that's both equal and unequal to itself, no matter what the equality code does)

tommilligan commented 7 years ago

Yep - writing up a pull request now

tommilligan commented 7 years ago

When writing this I dindn't define __ne__ as !__eq__, as this may lead to evaluation of !NotImplemented (which gives False, rather than also NotImplemented).

Just checking now, and StackOverflow appears to agree they should not be nested - shame as it would be much simpler!

skorokithakis commented 7 years ago

I don't have time right now to read the post thoroughly, but it seems to indicate that if __ne__ raises NotImplementedError, Python will use the opposite of ==, no?

tommilligan commented 7 years ago

That wasn't the initial impression I got - but will go back and have another look. I think the way I've implemented it here at least works and fits in with the style of the module, but I will see if there are more pythonic idioms out there.

vaab commented 7 years ago

Please note that this does not happen on python 3. IMHO a fix (as proposed in #29) is to implement the missing behavior of python 2 exactly as it is done in python 3. What do you think of this ?

skorokithakis commented 7 years ago

The StackOverflow answer details the right way to do this:

    if sys.version_info[0] == 2:
        def __ne__(self, other):
            equal = self.__eq__(other)
            return equal if equal is NotImplemented else not equal

Although, I think an even better way would be this:

    if sys.version_info[0] == 2:
        def __ne__(self, other):
            equal = self == other
            return equal if equal is NotImplemented else not equal
vaab commented 7 years ago

@skorokithakis I've used the stackoverflow answer after having implemented it, but not as pythonic.

The problem is quite complex:

As I understand it, unstead making subtle choices here, I think it is better to stick to the actual python 3 behavior for matter or standard behavior. Which is: https://github.com/python/cpython/blob/3.6/Objects/typeobject.c#L3584-L3606

The C version seems to call __eq__ of self, but I don't really have time right now to check where is hooked this tp_richcompare.

So for now, I think we should stick to the first version as it is already in the proposed PR #29 .

vaab commented 7 years ago

I released 0.1.3 with this.

tommilligan commented 7 years ago

All looks good - interesting problem, I agree with your final solution of backporting Python 3 behaviour