Uh oh!
There was an error while loading. Please reload this page.
gh-121604: Raise DeprecationWarnings in importlib - #121765
Conversation
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
tomasr8
left a comment
There was a problem hiding this comment.
Thanks! I left you some comments ;)
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| @@ -1,5 +1,5 @@ | |||
| """The machinery of importlib: finders, loaders, hooks, etc.""" | |||
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Tomas R <tomas.roun8@gmail.com>
Thanks for the review Co-authored-by: Tomas R <tomas.roun8@gmail.com>
rashansmith
commented
Jul 14, 2024
@tomasr8 thanks for the review, I made some changes. |
| def __init__(self): | ||
| warnings.warn(f"Loader is deprecated.", | ||
| DeprecationWarning, stacklevel=2) | ||
| super().__init__() |
There was a problem hiding this comment.
I'm not 100% sure but I think we should raise the deprecation warning when the class is imported, not when you create a new instance with it. Going by the example in the original PEP 562 it'd look (I think) something like this:
# Rename the deprecated classclass_Loader(metaclass=abc.ABCMeta):
...
def__getattr__(name):
ifname=='Loader':
warnings.warn(f"The 'Loader' class is deprecated.",
DeprecationWarning, stacklevel=2)
return_LoaderraiseAttributeError(f"module {__name__!r} has no attribute {name!r}")There was a problem hiding this comment.
Ok, no problem. Thanks for the reference link! Another approach I had was to just put the warning message by itself in the first line under the class name. Would this have a similar affect? I removed it because I then saw the deprecation error when running the make command.
There was a problem hiding this comment.
I think the problem with the other approach is that you'll get the warning whenever you import the module, not just the deprecated class specifically. For example, import importlib.abc will produce a warning for InspectLoader even if you're not actually using it
| def __getattr__(name): | ||
| if name in ('DEBUG_BYTECODE_SUFFIXES', 'OPTIMIZED_BYTECODE_SUFFIXES', 'WindowsRegistryFinder'): |
There was a problem hiding this comment.
The first if statement could be omitted, just use the if-elif statement ;)
ifnamein ('DEBUG_BYTECODE_SUFFIXES', 'OPTIMIZED_BYTECODE_SUFFIXES'):
...
elifname=='WindowsRegistryFinder':
...Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Charlie Zhao <zhaoyu_hit@qq.com>
brettcannon
commented
Sep 26, 2024
FYI this PR breaks the test suite, so CI isn't passing. |
tomasr8
commented
Nov 26, 2024
Hi @rashansmith! Looks like this PR hasn't had any activity in a while, would you like to continue working on it? |
StanFromIreland
commented
Dec 15, 2024
Hello @rashansmith if you do not have the time to complete this I can take on the project. |
tomasr8
commented
Dec 15, 2024
Hey! Thanks for volunteering @StanFromIreland, but I'm already working on finishing this PR :/ If you still want to help with this though, I'd appreciate your review! I'll send the updated PR tomorrow 🙂 |
tungol
commented
Dec 16, 2024
This MR is currently adding deprecation warnings to |
rashansmith
commented
Dec 16, 2024
Hello everyone, thanks for taking this on. It was my first attempt at a PR in this repo so I'm looking forward to seeing how the work here is done and hopefully I can contribute to more work in the future! |
tomasr8
commented
Dec 18, 2024
Thanks @rashansmith for your work! I'm giving you credit in the followup PR :) I'll close this now since it's superseded by #128007 |
This fixes#121604.
The others defined in the issue can be considered already done.
Issue: #121604
DeprecationWarning#121604