GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-97850: Deprecate `find_loader` and `get_loader` in `pkgutil` by sobolevn · Pull Request #98520 · python/cpython

Conversation

@sobolevn

@sobolevn sobolevn commented Oct 21, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Refs #64138

  • Issue: Meta issue for cleaning up import system cruft #97850

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@sobolevn
pythongh-97850: Deprecate find_module and get_module in pkgutil
b7de6df
@bedevere-bot bedevere-bot added the awaiting review label Oct 21, 2022
@sobolevn sobolevn mentioned this pull request Oct 21, 2022
Meta issue for cleaning up import system cruft #97850
Open
25 of 31 tasks
@JelleZijlstra
JelleZijlstra requested a review from brettcannon October 26, 2022 03:52
merwok
merwok reviewed Nov 8, 2022
View reviewed changes
Comment thread Lib/pkgutil.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

CAM-Gerlach
CAM-Gerlach reviewed Nov 8, 2022
View reviewed changes

@CAM-Gerlach CAM-Gerlach left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Doc/whatsnew/3.12.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/pkgutil.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/pkgutil.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_pkgutil.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_pkgutil.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_pkgutil.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Misc/NEWS.d/next/Library/2022-10-21-16-23-31.gh-issue-97850.N46coo.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

CAM-Gerlach
CAM-Gerlach reviewed Nov 8, 2022
View reviewed changes
Comment thread Doc/whatsnew/3.12.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

sobolevn and others added 2 commits November 9, 2022 16:22
@sobolevn
Address review
b1e6d4d
@sobolevn @CAM-Gerlach
Update Misc/NEWS.d/next/Library/2022-10-21-16-23-31.gh-issue-97850.N4…
d55a9b7
…6coo.rst

Co-authored-by: C.A.M. Gerlach <[email protected]>
@sobolevn

sobolevn commented Nov 9, 2022

Copy link
Copy Markdown
Member Author

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! 🤝

CAM-Gerlach reacted with thumbs up emoji CAM-Gerlach reacted with heart emoji
All reactions
  • 👍 1 reaction
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

brettcannon
brettcannon requested changes Nov 25, 2022
View reviewed changes

@brettcannon brettcannon left a comment

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Lib/test/test_pkgutil.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot added awaiting changes and removed awaiting review labels Nov 25, 2022
@bedevere-bot

bedevere-bot commented Nov 25, 2022

Copy link
Copy Markdown

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@CAM-Gerlach

CAM-Gerlach commented Nov 26, 2022

Copy link
Copy Markdown
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).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

sobolevn added 2 commits November 26, 2022 13:13
@sobolevn
Use just one module
e282911
@sobolevn
Merge branch 'deprecate-pkgutil-get-find-loader' of https://github.co…
4f9a697
…m/sobolevn/cpython into deprecate-pkgutil-get-find-loader
@kdeldycke kdeldycke mentioned this pull request Jan 2, 2023
Python 3.12-dev & Windows: zipimporter has no attribute find_module pypa/pip#11688
Closed
1 task done
@sobolevn

sobolevn commented Jan 7, 2023

Copy link
Copy Markdown
Member Author

@brettcannon I forgot to send the magic phrase!

I have made the requested changes; please review again

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot added awaiting change review and removed awaiting changes labels Jan 7, 2023
@bedevere-bot

bedevere-bot commented Jan 7, 2023

Copy link
Copy Markdown

Thanks for making the requested changes!

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot
bedevere-bot requested a review from brettcannon January 7, 2023 08:41
@brettcannon
Merge branch 'main' into deprecate-pkgutil-get-find-loader
4222f23
brettcannon
brettcannon requested changes Jan 21, 2023
View reviewed changes

@brettcannon brettcannon left a comment

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Doc/whatsnew/3.12.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Misc/NEWS.d/next/Library/2022-10-21-16-23-31.gh-issue-97850.N46coo.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot added awaiting changes and removed awaiting change review labels Jan 21, 2023
@bedevere-bot

bedevere-bot commented Jan 21, 2023

Copy link
Copy Markdown

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sobolevn @brettcannon
Apply suggestions from code review
817f2da
Co-authored-by: Brett Cannon <[email protected]>
@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 and others added 2 commits February 5, 2023 14:59
@sobolevn @brettcannon
Update Misc/NEWS.d/next/Library/2022-10-21-16-23-31.gh-issue-97850.N4…
8b116d9
…6coo.rst

Co-authored-by: Brett Cannon <[email protected]>
@sobolevn
Merge branch 'main' into deprecate-pkgutil-get-find-loader
e94a6f3
@sobolevn

sobolevn commented Apr 11, 2023

Copy link
Copy Markdown
Member Author

I have made the requested changes; please review again 🙂

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot added awaiting change review and removed awaiting changes labels Apr 11, 2023
@bedevere-bot

bedevere-bot commented Apr 11, 2023

Copy link
Copy Markdown

Thanks for making the requested changes!

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot
bedevere-bot requested a review from brettcannon April 11, 2023 07:30
@sobolevn
Mention deprecation in pkgutil docs
e073fac
@sobolevn

sobolevn commented Apr 19, 2023

Copy link
Copy Markdown
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!

hugovk reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

hugovk
hugovk reviewed Apr 19, 2023
View reviewed changes
Comment thread Doc/library/pkgutil.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/library/pkgutil.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/whatsnew/3.12.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

sobolevn and others added 2 commits April 19, 2023 15:57
@sobolevn
Address review
1e26981
@arhadthedev
Merge branch 'main' into deprecate-pkgutil-get-find-loader
24badea
brettcannon
brettcannon reviewed May 3, 2023
View reviewed changes
Comment thread Lib/pkgutil.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/pkgutil.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@brettcannon
Apply suggestions from code review
619c4dc
brettcannon
brettcannon approved these changes May 3, 2023
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting change review labels May 3, 2023
@brettcannon

brettcannon commented May 3, 2023

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sobolevn
Merge branch 'main' into deprecate-pkgutil-get-find-loader
b65576a
@sobolevn

sobolevn commented May 3, 2023

Copy link
Copy Markdown
Member Author

Done :)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@brettcannon
brettcannon merged commit d6e83fb into python:main May 3, 2023
@bedevere-bot bedevere-bot removed the awaiting merge label May 3, 2023
@brettcannon

brettcannon commented May 3, 2023

Copy link
Copy Markdown
Member

Thanks!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@warsaw

warsaw commented May 4, 2023

Copy link
Copy Markdown
Member

Thank you for this additional cleanup!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Wulian233 Wulian233 mentioned this pull request Jun 16, 2024
Remove pkgutil.get_loader and pkgutil.findloader #120581
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@merwok merwok merwok left review comments
@hugovk hugovk hugovk left review comments
@CAM-Gerlach CAM-Gerlach CAM-Gerlach left review comments
@brettcannon brettcannon brettcannon approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@sobolevn @bedevere-bot @CAM-Gerlach @brettcannon @warsaw @merwok @hugovk @arhadthedev
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.