bpo-29986: Delete tip to raise TypeError from tp_richcompare. #987
Conversation
I am not sure when TypeError is the right choice. Definitely, most of the time I've seen it done, it causes trouble, and `NotImplemented` usually does something better. For example, see the work in https://bugs.python.org/issue8743 to get set to interoperate correctly with other set-like classes --- a problem caused by the use of TypeError instead of returning NotImplemented (e.g. https://hg.python.org/cpython/rev/3615cdb3b86d). This advice seems to conflict with the usual and expected behavior of objects from Python: e.g. object().__lt__(1) returns NotImplemented rather than raising TypeError, despite < not "making sense" for object. Similarly for file objects and other uncomparable classes. Even complex numbers only return NotImplemented! >>> 1j.__lt__(1j) NotImplemented OTOH, if this note should be kept, and this PR rejected, this section could use a decent explanation of the difference between "undefined" (should return NotImplemented) and "unsupported" (should apparently raise TypeError). Perhaps a reference to an example from the stdlib.
|
@ssbr, thanks for your PR! By analyzing the history of the files in this pull request, we identified @birkenfeld, @1st1 and @vadmium to be potential reviewers. |
|
Thanks for the PR, but please file an issue on bugs.python.org to discuss the patch. |
ssbr
changed the title
|
|
||
| If you want to implement a type for which only a limited set of | ||
| comparisons makes sense (e.g. ``==`` and ``!=``, but not ``<`` and | ||
| friends), directly raise :exc:`TypeError` in the rich comparison function. |
There was a problem hiding this comment.
Instead of removing this note, maybe you should change it say return NotImplemented instead.
There was a problem hiding this comment.
Unfortunately that clashes with guidance provided on the issue. :) Maybe this can get hashed out there?
There was a problem hiding this comment.
Unfortunately that clashes with guidance provided on the issue
The important thing is that the bad advice (raise TypeError) is removed. The consensus is to return NotImplemented in this case. How explicit you want to make that in the docs is bikeshedding territory.
In other words: I'm totally fine with just removing that paragraph.
|
@ericsnowcurrently, can you take a look at this change? You recently modified this section of the docs (which is the merge conflict), but you didn't change this part, so your input may be helpful. Thanks! |
|
Unfortunately, I am not familiar enough right now with the relevant code to make a helpful review. Sorry. |
|
FWIW, to fix the merge conflict the relevant note just needs to be removed from wherever it is now in the file. :) |
|
(I forgot about this PR; don't use github much and didn't notice the replies.) Is this blocked on me? |
|
Closing and reopening to retry failed CI job. |
|
@ssbr Would you please rebase this on master? The tests won't pass due to an expired SSL certificate (unrelated to your PR). |
I am not sure when
TypeErroris the right choice. Definitely, most of the time I've seen it done, it causes trouble, andNotImplementedusually does something better.For example, see the work in https://bugs.python.org/issue8743 to get set to interoperate correctly with other set-like classes --- a problem caused by the use of
TypeErrorinstead of returningNotImplemented(e.g. https://hg.python.org/cpython/rev/3615cdb3b86d).This advice seems to conflict with the usual and expected behavior of objects from Python: e.g.
object().__lt__(1)returnsNotImplementedrather than raisingTypeError, despite<not "making sense" forobject. Similarly for file objects and other uncomparable classes. Even complex numbers only returnNotImplemented!OTOH, if this note should be kept, and this PR rejected, this section could use a decent explanation of the difference between "undefined" (should return
NotImplemented) and "nonsensical" (should apparently raiseTypeError). Perhaps a reference to an example from the stdlib.https://bugs.python.org/issue29986
Automerge-Triggered-By: @tiran