Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: dev: added inequality method (fixes #26) #28

Closed
wants to merge 1 commit into from

Conversation

tommilligan
Copy link

This fixes issue #26 where checking if two Color objects were not equal (using !=) always returned True.

PR includes tests. I don't think additional changes to documentation are required.

This PR is based on the master branch, as the dev branch is currently failing build for Python 3.4.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 98.729% when pulling 6f9167a on tommilligan:master into 69f5add on vaab:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 98.729% when pulling 6f9167a on tommilligan:master into 69f5add on vaab:master.

@tommilligan
Copy link
Author

Root cause was that the Color class already has a custom __eq__ method, but no custom __ne__ method.

This PR:

  • implements the __ne__ method
  • refactors the __eq__ method
  • adds relevant doctests

@vaab
Copy link
Owner

vaab commented Apr 8, 2017

Ok, many thanks. I'm in the process of reviewing your code and going into the #25 cases you've brought up.
As a first notice, I'm not very keen upon adding this _is_comparable(..) method, and I'll look into enforcing python 3 default behavior as I understand it (which should be to use not ==).

I have posted another PR #29, feel free to comment !

@vaab
Copy link
Owner

vaab commented Apr 11, 2017

I pushed the other PR #29 that tackle the same issue a little differently. If anything is wrong with that, please voice your concern.
Thanks !

@vaab vaab closed this Apr 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants