Instance is now checked on other during __eq__ to allow for other.__eq__...

[sayeghr]

Instance is now checked on other during eq to allow for other.eq() a chance of being executed. This fixes a bug where it raises an error when attempting to compare a Color object with None.

This fixes a bug when the Color class is used in Sqlalchemy-Utils and a Color object is compared with a Null value in the database.

[vaab]

This makes sense. I was wondering if we'd like to tackle this in the equality callable to let more choices to the user of the library. But I'm not sure this is a big deal. And I'm inclined to merge this quite quickly.

Do not bother about Travis saying it doesn't pass the test. They have updated recklessly their VM infrastructure to some very recent version of setuptools which breaks d2to1. The bug was reported.

I'll take care of that.

Thanks for your PR. I should include it soon.

[sayeghr]

Happy to help!

[vaab]

Hmm, I was in the process of reviewing it, and I dislike the fact that you are using an Exception as a return value, and actually I don't understood why you do that. I guess I'll move the implementation in the equality relation, or simply return False. I'd like to make sure although that I'll address correctly your issue, and I'm now unsure to have correctly understood your concerns. Would you be able to show me a simple test that fails with the current master and that passes with your corrections ? That would clarify more what you need.

[sayeghr]

Vaab,

The proposed eq method does not return an exception, it returns the NotImplemented singleton. This singleton is used to signal that the classes eq method cannot be used for the current comparison, since the two objects being compared do not belong to the same instance. This allows for the other objects eq to be called instead.

See the following URL for an explanation of the NotImplemented singleton: http://shahriar.svbtle.com/python-notimplemented-type

Your code breaks in the following example:

newColor = Color() newColor == None (EXCEPTION) newColor == "Hello" (EXCEPTION)

This is because the current eq compares self.hex with other.hex , but it is not safe to assume that the other object has a hex attribute.

With the new example, the above will try newColor.eq, get the NotImplemented singleton since they are not the same instance, then it will use the None singleton's eq that presumably checks if the two objects share the same reference (Not sure what None's eq looks like TBH).

[sayeghr]

This really is the correct way to handle the eq so that your class plays well with others.

[vaab]

You are definitively right, sorry about that. I didn't know about the specifics of NotImplemented (which I obviously mistake for NotImplementedError exception). I've learned something new, many thanks.

I've pushed a new version of your commit (mostly some tests and docs, and formatted the commit message). Can I ask you to take a quick glance and check I've correctly documented and tested your issue ?

[sayeghr]

Vaab, I read through your documentation. Why would it be the case that red == blue would resolve as True?

They are both Color objects with different hex values?

+Note that these instance would compare as equal to any other color:: +

• red == blue

• True +

If both objects are Color, it will use your lambda function that compares self.hex to other.hex.

[vaab]

Should I add the following bold text to make it clearer ?

As a side note, whatever your custom equaltiy function is, it won't be	used if you compare to anything else than a Colour instance::
>>> red = Color("red", equality=lambda c1, c2: True)
>>> blue = Color("blue", equality=lambda c1, c2: True)
Note that with such equality method, any color will compare equal to any other color... **which is
pretty useless as is, but nice to showcase what follows, so don't be astonished if:**
>>> red == blue
True

[sayeghr]

Ah, okay I understand. You changed the equality function of those colour objects. Yes, sure that's fine, I suppose.