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
base: main
Are you sure you want to change the base?
Conversation
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.
Misc/NEWS.d/next/Library/2022-10-21-16-23-31.gh-issue-97850.N46coo.rst
Outdated
Show resolved
Hide resolved
…6coo.rst Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
|
Thank you for the reviews: @merwok and @CAM-Gerlach I've also checked that >>> 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>
>>>
I've also merged one of the @CAM-Gerlach's suggestion, so you get a credit for your awesome proposals! |
Lib/test/test_pkgutil.py
Outdated
| @@ -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"]: | |||
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.
Why the specific check for multiple modules? Any reason to not just import any module and check the warning appears? Same with below.
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.
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!
|
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 |
Just FYI, you can apply some or all of them at once by going to the |
…m/sobolevn/cpython into deprecate-pkgutil-get-find-loader
Refs #64138