Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

gh-118761: Add test_lazy_import for more modules#133057

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
JelleZijlstra merged 9 commits intopython:mainfromdanielhollas:test-lazy-import
May 5, 2025

Conversation

danielhollas
Copy link
Contributor

@danielhollasdanielhollas commentedApr 27, 2025
edited
Loading

I went through my past PRs where I sped up the import time ofthreading andpathlib modules (#114509 and#123520) and added regression tests to ensure that the respective module imports stay lazy, using@JelleZijlstra's newensure_lazy_import test fixture in#132614.

Happy to do this for more modules if it is better to do it in one PR instead of many.

I wasn't quite sure where to put these tests so happy to take guidance on that.

@danielhollas
Copy link
ContributorAuthor

@Wulian233 I am not sure what you mean. Please see#132614 and#118761 (comment) for the motivation.

JelleZijlstra reacted with thumbs up emoji

@JelleZijlstra
Copy link
Member

Thanks! Feel free to make the same change for more modules in this PR.

@danielhollas
Copy link
ContributorAuthor

@JelleZijlstra thanks for taking a look! I've added the tests forenum,functools andemail.utils modules. This should cover the modules handled in the original issue#109653. I'll stop for now.

Unless somebody else beats me to it, I can go through the rest of the modules handled in#118761, maybe next weekend.

JelleZijlstra reacted with heart emoji

@danielhollasdanielhollas changed the titlegh-118761: test_lazy_import for threading and pathlib modulesgh-118761: Add test_lazy_import for more modulesApr 28, 2025
@rhettingerrhettinger removed their request for reviewApril 29, 2025 16:07
@danielhollas
Copy link
ContributorAuthor

Unless somebody else beats me to it, I can go through the rest of the modules handled in#118761, maybe next weekend.

I went through the rest of the modules from#118761 and added the respective tests.
I only skippedimportlib.metadata andtomllib since it is not clear to me if the tests should live in cpython repo or in the respective upstream repositories. Leaving that discussion for a follow-up PR.

This should be good to go.@JelleZijlstra I pushed one more commit which I forgot to push with the others, apologies.

Copy link
Member

@JelleZijlstraJelleZijlstra left a comment

Choose a reason for hiding this comment

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

Found a few more

@JelleZijlstra
Copy link
Member

Also a merge conflict. If you don't get to it I can spend a bit more time later today fixing it up, hopefully we can still get it in by the beta. (And it's not the end of the world if we don't.)

danielhollasand others added2 commitsMay 5, 2025 22:14
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
@danielhollas
Copy link
ContributorAuthor

Also a merge conflict. If you don't get to it I can spend a bit more time later today fixing it up, hopefully we can still get it in by the beta. (And it's not the end of the world if we don't.)

Done, thanks for a thorough review!

I'm getting these by searching for "import" in the source file and collecting the imports inside functions

Ah, good thinking. I've only included those modules that were specifically made lazy in the various PRs.

This got me thinking though that the current approach is somewhat suboptimal -- it would be more robust to have an allowlist of modules. Right now, a previously unused module can still be added and the tests might still pass. But this PR is definitely an improvement, LMK if you want me to open a follow-up issue for discussion.

@JelleZijlstra
Copy link
Member

An allowlist could make sense too, but might be harder to maintain. Feel free to open an issue discussing it. I guess my primary motivation was "I did all this work to make it so typing doesn't import annotationlib, let's make it so we don't regress by accident".

danielhollas reacted with thumbs up emoji

@JelleZijlstraJelleZijlstraenabled auto-merge (squash)May 5, 2025 22:37
@JelleZijlstraJelleZijlstra merged commitcae660d intopython:mainMay 5, 2025
41 of 42 checks passed
@danielhollasdanielhollas deleted the test-lazy-import branchMay 5, 2025 23:00
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@JelleZijlstraJelleZijlstraJelleZijlstra approved these changes

@barneygalebarneygaleAwaiting requested review from barneygalebarneygale is a code owner

@ethanfurmanethanfurmanAwaiting requested review from ethanfurmanethanfurman is a code owner

@Eclips4Eclips4Awaiting requested review from Eclips4Eclips4 is a code owner

Assignees
No one assigned
Labels
skip newstestsTests in the Lib/test dir
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

3 participants
@danielhollas@JelleZijlstra@picnixz

[8]ページ先頭

©2009-2025 Movatter.jp