Skip to content

gh-146401: suggest a module in sys.builtin_module_names#146421

Open
Locked-chess-official wants to merge 5 commits intopython:mainfrom
Locked-chess-official:suggest_builtin_module
Open

gh-146401: suggest a module in sys.builtin_module_names#146421
Locked-chess-official wants to merge 5 commits intopython:mainfrom
Locked-chess-official:suggest_builtin_module

Conversation

@Locked-chess-official
Copy link
Copy Markdown
Contributor

@Locked-chess-official Locked-chess-official commented Mar 25, 2026 •
edited by bedevere-app bot
Loading

Change to use @classmethod to yield specs.

@bedevere-app bedevere-app bot mentioned this pull request Mar 25, 2026
Open
picnixz
picnixz requested changes Mar 25, 2026
View reviewed changes
Lib/importlib/_bootstrap.py Outdated
Comment on lines +941 to +946
@classmethod
def discover(cls, spec=None):
if spec is not None: # assume that built-in modules have no submodule
return
for i in sys.builtin_module_names:
yield cls.find_spec(i)
Copy link
Copy Markdown
Member

@picnixz picnixz Mar 25, 2026

Choose a reason for hiding this comment

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

I don't understand why we need this specific change here. It feels wrong to change importlib.

@bedevere-app
Copy link
Copy Markdown

bedevere-app bot commented Mar 25, 2026

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.

@picnixz
Copy link
Copy Markdown
Member

picnixz commented Mar 25, 2026

If you want to improve suggestions for built-in names, it should be at the REPL level. Not in importlib. Please update the issue about your rationale of this change first.

@picnixz picnixz closed this Mar 25, 2026
@picnixz
Copy link
Copy Markdown
Member

picnixz commented Mar 25, 2026

Re-opening as we will make the change elsewhere.

@picnixz picnixz reopened this Mar 25, 2026
@picnixz picnixz marked this pull request as draft March 25, 2026 14:51
@picnixz
Copy link
Copy Markdown
Member

picnixz commented Mar 25, 2026

@Locked-chess-official I'll make it a draft. Once you're done with your changes, ping me and I'll remove the reviewers as they won't be relevant anymore.

@picnixz picnixz marked this pull request as ready for review March 25, 2026 16:09
picnixz
picnixz reviewed Mar 25, 2026
View reviewed changes
vstinner
vstinner reviewed Mar 25, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

Can you add a test on this change?

@vstinner
Copy link
Copy Markdown
Member

vstinner commented Mar 26, 2026

I added "skip news" label since I'm not sure that it's useful to advertize this change. There is https://docs.python.org/dev/whatsnew/3.15.html#whatsnew315-improved-error-messages but Python 3.14 already makes suggestions on import typo, it's just that it didn't take built-in modules in account. Tell me if you disagree :-)

vstinner
vstinner approved these changes Mar 26, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM. It makes sense to also make suggestions on built-in modules.

@picnixz
Copy link
Copy Markdown
Member

picnixz commented Mar 26, 2026

Yes, it makes sense but I actually don't know whether adding a discover method to BuiltinImporter makes better sense or whether it makes sense to yield the builtin names directly from sys.*, hence my question to Brett.

I wasn't fond of changing BuiltinImporter but traceback.py relies on discover() to get the module names from each finder in sys.meta_path (which BuiltinImporter is one of) so I was worried about downstream changes if we were to add this method.

vstinner
vstinner reviewed Apr 2, 2026
View reviewed changes
@Locked-chess-official @vstinner
Update Lib/importlib/_bootstrap.py
1621018 
Co-authored-by: Victor Stinner <vstinner@python.org>
vstinner
vstinner approved these changes Apr 2, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM. I'm also fine with the importer discover() approach. @FFY00 prefers the discover() approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@brettcannon brettcannon brettcannon left review comments

@picnixz picnixz picnixz requested changes

@FFY00 FFY00 FFY00 left review comments

@vstinner vstinner vstinner approved these changes

Assignees

No one assigned

Labels

awaiting merge skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Locked-chess-official @picnixz @vstinner @brettcannon @FFY00