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_loader and get_loader in pkgutil #98520

Merged
merged 14 commits into from
May 3, 2023

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

Choose a reason for hiding this comment

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

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
Doc/whatsnew/3.12.rst Outdated Show resolved Hide resolved
sobolevn and others added 2 commits November 9, 2022 16:22
…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

Choose a reason for hiding this comment

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

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

Lib/test/test_pkgutil.py Outdated Show resolved Hide resolved
@bedevere-bot
Copy link

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

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).

@sobolevn
Copy link
Member Author

sobolevn commented Jan 7, 2023

@brettcannon I forgot to send the magic phrase!

I have made the requested changes; please review again

@bedevere-bot
Copy link

Thanks for making the requested changes!

@brettcannon: please review the changes made to this pull request.

Copy link
Member

@brettcannon brettcannon left a comment

Choose a reason for hiding this comment

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

Unfortunately I just realized that the documentation for pkgutil needs updating to mention the deprecations.

Doc/whatsnew/3.12.rst Outdated Show resolved Hide resolved
@bedevere-bot
Copy link

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.

Co-authored-by: Brett Cannon <brett@python.org>
@sobolevn sobolevn changed the title gh-97850: Deprecate find_module and get_module in pkgutil gh-97850: Deprecate find_loader and get_loader in pkgutil Feb 5, 2023
@sobolevn
Copy link
Member Author

I have made the requested changes; please review again 🙂

@bedevere-bot
Copy link

Thanks for making the requested changes!

@brettcannon: please review the changes made to this pull request.

@sobolevn
Copy link
Member Author

Oups, just found out that my push with pkgutil.rst change did not make it into this branch :(

Redid it now, it worked!

Doc/library/pkgutil.rst Outdated Show resolved Hide resolved
Doc/library/pkgutil.rst Outdated Show resolved Hide resolved
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
@brettcannon
Copy link
Member

@sobolevn the changes look good, but now there are unfortunately merge conflicts.

@sobolevn
Copy link
Member Author

sobolevn commented May 3, 2023

Done :)

@brettcannon brettcannon merged commit d6e83fb into python:main May 3, 2023
@brettcannon
Copy link
Member

Thanks!

@warsaw
Copy link
Member

warsaw commented May 4, 2023

Thank you for this additional cleanup!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants