Fix comparison #104
Fix comparison #104
Conversation
semver.py
Outdated
| def _ensure_is_comparable(other): | ||
| comparable_types = (VersionInfo, dict, tuple) | ||
| if not isinstance(other, comparable_types): | ||
| raise NotImplementedError("other type %r must be in %r" |
There was a problem hiding this comment.
https://docs.python.org/3/library/constants.html#NotImplemented
You need to return NotImplemented here. Python itself will raise proper TypeError if objects aren't comparable.
There was a problem hiding this comment.
semver.py
Outdated
| @@ -261,6 +257,13 @@ def _compare_by_keys(d1, d2): | |||
| return rccmp | |||
|
|
|||
|
|
|||
| def _ensure_is_comparable(other): | |||
| comparable_types = (VersionInfo, dict, tuple) | |||
There was a problem hiding this comment.
@ppkt made a good point about using interfaces in place of dict/tuple. This will allow us to compare with dict-like / tuple-like objects for free (well, not really, but nothing to care about).
The only thing is str - it's Iterable, but unlikely *str will give us what we really expected from it.
| return NotImplemented | ||
| if not _is_comparable(other): | ||
| if PY2: | ||
| raise TypeError |
There was a problem hiding this comment.
Hm, but why? NotImplemented existed for Python 2.x as well and there rules are the same for it.
There was a problem hiding this comment.
See previous build https://travis-ci.org/k-bx/python-semver/builds/404887965?utm_source=github_status&utm_medium=notification for Python 2.7 (see job)
even if NotImplemented existed for Python 2.x returning it doesn't raise TypeError (contrary to Python 3.x) and comparison wasn't returning a correct result.
There was a problem hiding this comment.
No, NotImplemented in Py2.7 works in the same way: https://docs.python.org/2.7/library/constants.html#NotImplemented
There was a problem hiding this comment.
Ah, Python 2.x swallows them and returns always True or False...or False in our case.
There was a problem hiding this comment.
I think, it would be better to move that check and behaviour difference into tests.
While it's good idea to keep same behaviour in both Pythons, Python 2.x world tells that every comparison ends with boolean result, so we would looks here like a rule breaker.
Raising TypeError for Python 2.x also breaks NotImplemented behavior when if a.__eq__(b) fails, Python tries to do b.__eq__(a) - by raising exception we drops the second and our behaviour in both Pythons still remains inconsistent.
Closes #102
Closes #103