Skip to content

Conversation

@rashansmith
Copy link
Contributor

@rashansmithrashansmith commented Jul 14, 2024

@ghost
Copy link

ghost commented Jul 14, 2024

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-app
Copy link

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 skip news label instead.

Copy link
Member

@tomasr8tomasr8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I left you some comments ;)

@@ -1,5 +1,5 @@
"""The machinery of importlib: finders, loaders, hooks, etc."""

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd keep the newline

blurb-itbotand others added 4 commits July 14, 2024 12:46
Co-authored-by: Tomas R <tomas.roun8@gmail.com>
 Thanks for the review Co-authored-by: Tomas R <tomas.roun8@gmail.com>
@rashansmith
Copy link
ContributorAuthor

@tomasr8 thanks for the review, I made some changes.

Comment on lines +10 to +13
def__init__(self):
warnings.warn(f"Loader is deprecated.",
DeprecationWarning, stacklevel=2)
super().__init__()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}")

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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):
ifnamein ('DEBUG_BYTECODE_SUFFIXES', 'OPTIMIZED_BYTECODE_SUFFIXES', 'WindowsRegistryFinder'):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first if statement could be omitted, just use the if-elif statement ;)

ifnamein ('DEBUG_BYTECODE_SUFFIXES', 'OPTIMIZED_BYTECODE_SUFFIXES'): ... elifname=='WindowsRegistryFinder': ...

Co-authored-by: Charlie Zhao <zhaoyu_hit@qq.com>
@ambvambv changed the title [3.14] Raise DeprecationWarnings (GH-121604) gh-121604: Raise DeprecationWarnings in importlibJul 16, 2024
@brettcannon
Copy link
Member

FYI this PR breaks the test suite, so CI isn't passing.

@tomasr8
Copy link
Member

Hi @rashansmith! Looks like this PR hasn't had any activity in a while, would you like to continue working on it?

@StanFromIreland
Copy link
Member

Hello @rashansmith if you do not have the time to complete this I can take on the project.

@tomasr8
Copy link
Member

Hello @rashansmith if you do not have the time to complete this I can take on the project.

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
Copy link
Contributor

This MR is currently adding deprecation warnings to importlib.abc.Loader, importlib.machinery.SourcelessFileLoader, and importlib.abc.InspectLoader, but none of those are currently documented as being deprecated; just the load_module method for each.

@rashansmith
Copy link
ContributorAuthor

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
Copy link
Member

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

@tomasr8tomasr8 closed this Dec 18, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make sure all deprecated items in importlib raise DeprecationWarning

6 participants

@rashansmith@brettcannon@tomasr8@StanFromIreland@tungol@CharlieZhao95