GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — gh-118803: Make `ByteString` deprecations louder; remove `ByteString` from `typing.__all__` and `collections.abc.__all__` by AlexWaygood · Pull Request #139127 · python/cpython Conversation
|
|
🤖 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. |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
| 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.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
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
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
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...
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
-
👍 1 reaction
|
The buildbots report a number of reference leaks on this branch, but I can reproduce them all locally on |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Reviewers
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assignedLabels
Projects
Milestone
No milestoneDevelopment
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
3 participants
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.