gh-146401: add BuiltinImporter.discover() to find built-in modules#146421
gh-146401: add BuiltinImporter.discover() to find built-in modules#146421Locked-chess-official wants to merge 5 commits intopython:mainfrom
BuiltinImporter.discover() to find built-in modules#146421Conversation
Locked-chess-official
requested review from
FFY00,
brettcannon,
ericsnowcurrently,
ncoghlan and
warsaw
as code owners
| @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) |
There was a problem hiding this comment.
I don't understand why we need this specific change here. It feels wrong to change importlib.
|
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 |
|
If you want to improve suggestions for built-in names, it should be at the REPL level. Not in |
|
Re-opening as we will make the change elsewhere. |
|
@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. |
Locked-chess-official
force-pushed
the
suggest_builtin_module
branch
from
3bdbfa1 to
c01925d
Compare
picnixz
removed request for
FFY00,
brettcannon,
ericsnowcurrently,
ncoghlan and
warsaw
vstinner
left a comment
vstinner
left a comment
There was a problem hiding this comment.
Can you add a test on this change?
… traceback test?)
|
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
left a comment
vstinner
left a comment
There was a problem hiding this comment.
LGTM. It makes sense to also make suggestions on built-in modules.
|
Yes, it makes sense but I actually don't know whether adding a I wasn't fond of changing |
This reverts commit c01925d. Changes to be committed: modified: Lib/traceback.py
Co-authored-by: Victor Stinner <vstinner@python.org>
picnixz
changed the title
BuiltinImporter.discover() to find built-in modules
| return False | ||
|
|
||
| @classmethod | ||
| def discover(cls, spec=None): |
There was a problem hiding this comment.
Can you also add tests for that? and a documentation entry as well? or maybe let's ask @FFY00 if he wants to make this API documented (if not, we can rename the PR to "Include built-in modules in traceback suggestions")
| if mod_spec := cls.find_spec(name): | ||
| yield mod_spec | ||
|
|
There was a problem hiding this comment.
Are other discover() methods meant to return a list or an iterator
5 participants
Change to use
@classmethodto yield specs.