bpo-35712: Make using NotImplemented in a boolean context issue a dep… #13195
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
serhiy-storchaka
approved these changes
May 8, 2019
|
The Azure Pipelines test failure was due to some unrelated issue with test_httplib and a bad cert. Looks like master fixed this in the past half hour, so I merged and pushed the new code to avoid the automated test failure. |
markshannon
reviewed
May 8, 2019
|
Could your please update the PR? Resolve conflicts and retarget this change to 3.9. I am going to merge this PR immediately after updating. It is too later for 3.8, but we have a lot of time to test it in 3.9. |
MojoVampire
force-pushed
the
fix-issue-35712
branch
from
33a1802 to
170346f
Compare
Hoping I did it correctly; a merge covering hundreds of commits seemed wrong, so I did a rebase and force push; only conflicts were in the 3.8 What's New (which I removed, before making a new commit adding the same info to the 3.9 What's New), and in object.c (where tp_reserved got renamed to tp_as_async in the comments near where I'd defined tp_as_number for NotImplemented; kept the new name). Otherwise rebased with minimal annoyance. |
MojoVampire
force-pushed
the
fix-issue-35712
branch
from
170346f to
f309a02
Compare
| @@ -146,6 +146,12 @@ Deprecated | |||
| of Python. For the majority of use cases users can leverage the Abstract Syntax | |||
| Tree (AST) generation and compilation stage, using the :mod:`ast` module. | |||
|
|
|||
| * Using :data:`NotImplemented` in a boolean context has been deprecated, | |||
| as it is almost exclusively the result of incorrect rich comparator | |||
| implementations. It will be made a :exc:`TypeError` in a future version | |||
There was a problem hiding this comment.
I am not sure that it will be a TypeError. It may be a RuntimeWarning.
The programmer can silence a warning programmatically if it is raised in the code which he cannot change, but he cannot silence a TypeError.
There was a problem hiding this comment.
Sorry, I didn't see this comment before pushing my changes to datamodel/constants, so they still reference TypeError.
In any event, is there some reason to stop at RuntimeWarning? I thought DeprecationWarning was supposed to give all the "code you cannot change" (third party packages) time to update themselves so nothing is using the error-prone code path by the time it becomes an actual error?
I can change it, but I'd prefer this to be an error eventually; RuntimeWarning is used in a few places, so the guidelines aren't clear, but in most cases it seems to boil down to either a mostly harmless thing where the warning tells you you asked Python to do something nonsensical/suboptimal (e.g. bad buffering configuration), and it chose to ignore you and do something valid/more optimal instead, or in the case of asyncio, a case that is probably an error (failing to await a coroutine), but the problem can't be detected until an unpredictable time after the problem occurred, so raising an exception isn't feasible at that point (the same way exceptions in del are suppressed). In most of these cases, Python can stumble along and ignoring the warning doesn't leave you doing anything wrong, just possibly suboptimal. In almost all cases, if NotImplemented is used in a boolean context, and there is no clearly valid way for Python to handle it (what should have delegated or raised TypeError becomes a return of True or False), thus my preference for (eventually) making it an error.
…recation, explicitly discourage using it in a boolean context.
|
@serhiy-storchaka, I believe @MojoVampire made all the requested changes and had an outstanding question for you in the last comment. Hopefully this can be merged soon. |
serhiy-storchaka
approved these changes
Mar 3, 2020
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…recation warning
A few tests and standard library functions were using it in boolean context (the standard library usage was technically correct, but obscure to the point of code smell, and easy to fix) while one test was using it on the assumption that it had not defined any methods beyond what
objectprovides (and the change gives it__bool__) so I tweaked them as necessary (making the standard lib code more clear and using other sentinels likeEllipsisorobject()for the relevant tests).https://bugs.python.org/issue35712