bpo-37685: Fixed __eq__, __lt__ etc implementations in some classes. #14952
Conversation
They now return NotImplemented for unsupported type of the other operand.
There was a problem hiding this comment.
I think we should probably not backport this all the way to 3.7 because it changes the semantics of comparisons (albeit in a good way).
If there is some subset of the changes that either just brings the C and Python implementations into agreement or fixes something that is much more obviously a bug, I think maybe we can split this into two PRs - one to be backported and one for 3.9 (and maybe 3.8 - I don't know the rules on that one, but from a practical purpose I think people treat 3.8 as sufficiently unstable that it's fine).
Lib/test/datetimetester.py
Outdated
| @@ -16,6 +16,7 @@ | |||
| import re | |||
| import struct | |||
| import unittest | |||
| from unittest.mock import ANY | |||
There was a problem hiding this comment.
We have a dedicated class for this in datetimetester.py to avoid relying on the implementation details of ANY.
I think we can move it into tests.support if you are going to be adding tests like this across multiple modules.
There was a problem hiding this comment.
I do not think we need yet one ANY in private tests.support if we can use public unittest.mock.ANY.
There was a problem hiding this comment.
unittest.mock.ANY only accidentally has the property that it compares equal to things. Even if that were a guaranteed property (it probably won't change, but it's an implementation detail), it's still confusing to see people using the "wild card for arguments" singleton from unittest.mock for one of its incidental properties (comparing equal to things).
If someone tried to understand this test and looked it up in the documentation, they would be confused as to what it means. I think it is much clearer to use a dedicated class for this (particularly since we already have one in the datetime tests.
There was a problem hiding this comment.
That class was added 15 days ago. Perhaps the author was not aware of mock.ANY. There are explicit tests for this property of mock.ANY, it is not incidental.
It is not very difficult to add a duplicate in test.support, but I am not sure that it is worth.
There was a problem hiding this comment.
There are explicit tests for this property of
mock.ANY, it is not incidental.
I don't know about that, people test implementation details all the time.
That class was added 15 days ago. Perhaps the author was not aware of
mock.ANY.
Definitely they were. The original PR used mock.ANY and I asked for it to be replaced with a dedicated class. You can see my comment on the original PR.
If you look at the documentation for ANY, it is clearly documented as a wild card to be used for making assertions about some calls to a function but not others. Nothing in the documentation says that it is implemented with a permissive __eq__, and in fact a permissive __eq__ is not always sufficient, as we learned in #14700. The ANY class does have the property you want, but it's a dedicated singleton with a semantic meaning other than "this compares equal to anything".
For reasons of clarity, I think it makes sense to have dedicated singletons for each of the custom comparison behaviors, even if there are other objects that also have those properties for their own reasons.
There was a problem hiding this comment.
This change has been extracted into #14996. Please take a look.
Lib/datetime.py
Outdated
| @@ -739,25 +739,25 @@ def __le__(self, other): | |||
| if isinstance(other, timedelta): | |||
| return self._cmp(other) <= 0 | |||
| else: | |||
| _cmperror(self, other) | |||
| return NotImplemented | |||
There was a problem hiding this comment.
Heh, we recently changed these and I guess I just assumed that _cmperror returned NotImplemented.
I agree with this change, but I would argue that it precludes backporting to 3.7 (and possibly 3.8), because it is a pretty significant change to the semantics of the language.
There was a problem hiding this comment.
This change fixes a discrepancy between Python and C implementations. Other changes are discussable, but this change is a bugfix and it should be backported.
There was a problem hiding this comment.
Yes, if that is the case, I agree about backporting the subset of these changes that fixes discrepancies between the Python and C implementations. We should figure out what that subset is and break it into two PRs - one for backporting and one for master (and maybe 3.8).
Lib/test/support/__init__.py
Outdated
| @@ -3093,6 +3093,21 @@ def __fspath__(self): | |||
| return self.path | |||
|
|
|||
|
|
|||
| @functools.total_ordering | |||
| class LargestObject: | |||
There was a problem hiding this comment.
How about making these singletons?
LargestObject = LargestObject() and SmallestObject = SmallestObject().
I don't think anyone needs to subclass these and there's no need to instantiate more than one of them.
Similarly we can move ComparesEqualClass into this module and make it a singleton.
|
When you're done making the requested changes, leave the comment: |
|
I agree that this probably shouldn't be backported as it's a shift in semantics. |
Would a backport to 3.8 still be appropriate since that branch is still in pre-release or should it exclusively be merged to 3.9? |
|
@aeros167 it's up to Serhiy and ultimately Lukasz as release manager, but beta is open to bugfixes, just not enhancements. |
…ythonGH-14952) They now return NotImplemented for unsupported type of the other operand.
|
Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8. |
|
Sorry, @serhiy-storchaka, I could not cleanly backport this to |
…sses. (pythonGH-14952) They now return NotImplemented for unsupported type of the other operand.. (cherry picked from commit 662db12) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
|
GH-17836 is a backport of this pull request to the 3.8 branch. |
…ythonGH-14952) They now return NotImplemented for unsupported type of the other operand.
…ythonGH-14952) They now return NotImplemented for unsupported type of the other operand.
They now return NotImplemented for unsupported type of the other operand.
https://bugs.python.org/issue37685