-
-
Notifications
You must be signed in to change notification settings - Fork 32.9k
gh-118803: Make ByteString
deprecations louder; remove ByteString
from typing.__all__
and collections.abc.__all__
#139127
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
gh-118803: Make ByteString
deprecations louder; remove ByteString
from typing.__all__
and collections.abc.__all__
#139127
Conversation
…tring` from `typing.__all__` and `collections.abc.__all__`
🤖 New build scheduled with the buildbot fleet by @AlexWaygood for commit fc787a5 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F139127%2Fmerge If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again. |
if attr == "ByteString": | ||
import warnings | ||
warnings._deprecated("collections.abc.ByteString", remove=(3, 17)) | ||
globals()["ByteString"] = _deprecated_ByteString |
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.
This means only the first time anyone does from collections.abc import ByteString
in a program will trigger a warning, right? That seems potentially problematic: maybe the first time is in some dependency you don't control, so you ignore the warning, and then it happens again in your code and you miss it.
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.
That's correct. I did that deliberately so that the warning wouldn't be too noisy, and since it's what we previously did for other modules such as ast
:
Lines 1897 to 1905 in d065edf
def __getattr__(name): | |
if name in _deprecated_globals: | |
globals()[name] = value = _deprecated_globals[name] | |
import warnings | |
warnings._deprecated( | |
f"ast.{name}", message=_DEPRECATED_CLASS_MESSAGE, remove=(3, 14) | |
) | |
return value | |
raise AttributeError(f"module 'ast' has no attribute '{name}'") |
But I'm happy to switch to it emitting the deprecation warning even on subsequent imports, if you prefer! It would simplify the implementation a little
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.
Probably OK to stick with the standard format here, hopefully it's enough to get people to notice...
The buildbots report a number of reference leaks on this branch, but I can reproduce them all locally on |
ByteString
fromtyping
andcollections.abc
#118803📚 Documentation preview 📚: https://cpython-previews--139127.org.readthedocs.build/