Skip to content
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-97850: Deprecate find_module and get_module in pkgutil #98520

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

sobolevn
Copy link
Member

@sobolevn sobolevn commented Oct 21, 2022

Lib/pkgutil.py Outdated Show resolved Hide resolved
Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Some comments and suggestions on the implementation of this deprecation, based on how we've handled others recently.

Pro tip (since not everyone seems to know this): if you go to the Files changed tab of this PR, you can apply some or all of the suggestions in one click by clicking Add to batch on each suggestion you want, and then click Commit either at the top or on any added suggestion with a descriptive message.

Doc/whatsnew/3.12.rst Outdated Show resolved Hide resolved
Lib/pkgutil.py Outdated Show resolved Hide resolved
Lib/pkgutil.py Outdated Show resolved Hide resolved
Lib/test/test_pkgutil.py Outdated Show resolved Hide resolved
Lib/test/test_pkgutil.py Outdated Show resolved Hide resolved
Lib/test/test_pkgutil.py Outdated Show resolved Hide resolved
sobolevn and others added 2 commits Nov 9, 2022
…6coo.rst

Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
@sobolevn
Copy link
Member Author

sobolevn commented Nov 9, 2022

Thank you for the reviews: @merwok and @CAM-Gerlach
It is much better now. I was not aware of ._deprecated helper.

I've also checked that get_loader does not get 2 warnings (because it is using find_loader inside):

>>> import pkgutil
>>> pkgutil.get_loader('os')
<stdin>:1: DeprecationWarning: 'pkgutil.get_loader' is deprecated and slated for removal in Python 3.14; use importlib.util.find_spec instead
<_frozen_importlib_external.SourceFileLoader object at 0x1012d55f0>
>>> 

test_pkgutil tests are also clean, no unwanted warnings. I will look through all logs again when it is completed.

I've also merged one of the @CAM-Gerlach's suggestion, so you get a credit for your awesome proposals! 🤝

Copy link
Member

@brettcannon brettcannon left a comment

Overall LGTM, but I think there might be some unnecessary checks in the tests for various module "kinds".

@@ -595,18 +590,28 @@ def test_get_loader_None_in_sys_modules(self):
del sys.modules[name]
self.assertIsNone(loader)

def test_get_loader_is_deprecated(self):
for module in ["sys", "os", "test.support"]:
Copy link
Member

@brettcannon brettcannon Nov 25, 2022

Choose a reason for hiding this comment

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

Why the specific check for multiple modules? Any reason to not just import any module and check the warning appears? Same with below.

Copy link
Member Author

@sobolevn sobolevn Nov 26, 2022

Choose a reason for hiding this comment

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

Old tests used to have these lines:

self.assertIsNotNone(pkgutil.find_loader("sys"))       self.assertIsNotNone(pkgutil.find_loader("os"))      self.assertIsNotNone(pkgutil.find_loader("test.support"))

So, I've copied the same behaviour.
Will address your review soon!

@bedevere-bot
Copy link

bedevere-bot commented Nov 25, 2022

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@CAM-Gerlach
Copy link
Member

CAM-Gerlach commented Nov 26, 2022

I've also merged one of the @CAM-Gerlach's suggestion, so you get a credit for your awesome proposals! handshake

Just FYI, you can apply some or all of them at once by going to the Files changed tab, clicking Add to batch on each suggestion and then clicking `Commit (with an appropriate message).

Sign up for free to 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.

None yet

5 participants