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
[3.7] bpo-33217: deprecate non-Enum lookups in Enums #6392
Conversation
Lookups such as `1 in Color` and `2 in SomeFlag()` will raise TypeError in 3.8+.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add the deprecated
directive(s ) in the module documentation and a note in the What's New document.
Lib/enum.py
Outdated
@@ -309,6 +310,11 @@ def __call__(cls, value, names=None, *, module=None, qualname=None, type=None, s | |||
return cls._create_(value, names, module=module, qualname=qualname, type=type, start=start) | |||
|
|||
def __contains__(cls, member): | |||
if not isinstance(member, Enum): | |||
warnings.warn( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add import warnings
just here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Lib/enum.py
Outdated
if not isinstance(member, Enum): | ||
warnings.warn( | ||
"using non-Enums in containment checks will raise " | ||
"TypeError in 3.8+", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"in Python 3.8"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"in 3.8" is a jargon. Non-core developers could not understand it. It is better to be more explicit in public warnings: "in Python 3.8".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, missed the "Python" part -- done now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good except that you should add an item about this to the Deprecated
section of the 3.7 whatsnew
document. Doc/whatsnew/3.7.rst => https://docs.python.org/3.7/whatsnew/3.7.html#deprecated
When you're done making the requested changes, leave the comment: |
I have made the requested changes; please review again. |
Thanks for making the requested changes! @serhiy-storchaka, @ned-deily: please review the changes made to this pull request. |
On 04/11/2018 04:03 PM, Ned Deily wrote:
***@***.**** approved this pull request.
Possibly a dumb question, but I can merge this in now?
…--
~Ethan~
|
Go for it! |
Thanks @ethanfurman for the PR 🌮🎉.. I'm working now to backport this PR to: 3.6. |
Sorry, @ethanfurman, I could not cleanly backport this to |
@ethanfurman Why would we backport this to 3.6? It would introduce a change in behavior late in its lifetime. |
Because 3.6 suffers from the same problems. If that's not a good idea that's fine.
|
Usually we don't backport deprecations, because this can break a code. The exception is 2.7 with -3 option. |
@ethanfurman Sure, 3.6 does but I think it would be worse to suddenly introduce a |
You need to port some changes to master: a news entry, docs changes, some tests. It would be easier to port the whole PR, and later convert a warning into an exception in a separate PR. |
@ned-deily, no problem. @serhiy-storchaka, are there directions somewhere on how to do that? I was just going to create a whole new PR with the changes in it (new docs, exceptions, etc.). |
@ethanfurman To clarify, I wouldn't object to a purely documentation change for 3.6.x that says that doing those kinds of containment tests will raise a You might be able to use Git's cherry-pick command as a starting point, probably with the -n option. Our workflow with git is sort of the reverse of what we were doing with hg, e.g. we now commit first to master than backport as necessary to maintenance branches. The |
Maybe using |
Lookups such as
1 in Color
and2 in SomeFlag()
will raise TypeErrorin 3.8+.
https://bugs.python.org/issue33217