◐ Shell
reader mode source ↗
Skip to content

gh-69605: PyREPL: insert "import" after "from foo <tab>"#148445

Open
loic-simon wants to merge 3 commits into
python:mainfrom
loic-simon:pyrepl-module-completion-insert-import
Open

gh-69605: PyREPL: insert "import" after "from foo <tab>"#148445
loic-simon wants to merge 3 commits into
python:mainfrom
loic-simon:pyrepl-module-completion-insert-import

Conversation

@loic-simon

@loic-simon loic-simon commented Apr 12, 2026 •
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Contributor

Two import completion improvements:

Input Current Proposed
from math <tab> from math from math import
from mat<tab> from math from math (extra space)

The first one should be pretty uncontroversial, since import is the only valid syntax here.

The second only change the behavior when a single match is found:

  • from cont<tab> still inserts from context (common prefix to contextlib and contextvar)
  • from foo<tab> still inserts nothing, since we have no match

It allows from mat<tab><tab> to insert from math import , which I find really smooth and pleasant to use!

Implementation notes: I settled to extend ImportParser.parse to also return a space_end boolean, and use that in ModuleCompleter.complete to return an import suggestion / add a space when needed.

This is a little more generic that needed for this change (eg. we don't really care abuout the space in import foo ), but it keep the parsing logic and completion logic orthogonal.

cc @tomasr8 (it'll have a few conflicts with your coloring PR, but I have no way to point this PR to some virtual "after your PR is merged" branch, have I?)

@loic-simon
PyREPL completion: insert "import" after "from foo " …
c33fefb 
Improve _pyrepl._module_completer.ModuleCompleter.get_completions:
  * "from x.y.z " [tab]-> "from x.y.z import "
  * "from foo"    [tab]-> "from foo "          [tab]-> "from foo import"
    (if only one suggestion)
@bedevere-app bedevere-app Bot mentioned this pull request Apr 12, 2026
Closed
@loic-simon loic-simon marked this pull request as ready for review April 12, 2026 13:34
tomasr8
tomasr8 reviewed Apr 12, 2026
View reviewed changes

@tomasr8 tomasr8 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

Thanks for the quick PR!

It allows from mat to insert from math import , which I find really smooth and pleasant to use!

This is quite nice indeed!

@johnslavik johnslavik self-requested a review April 12, 2026 20:30
@johnslavik

johnslavik commented Apr 12, 2026

Copy link
Copy Markdown
Member

I had the same idea a few days ago. Nice! I'll review soon.

@loic-simon

loic-simon commented Apr 12, 2026

Copy link
Copy Markdown
Contributor Author

Note: I juste found an existing bug (on main) while testing things 😅

>>> from math .<tab>
>>> from math .ath.integer  # instead of `from math .integer` (valid syntax!)

It's quite related to this change so I'm inclined to fix it in it's PR, if it's ok (and doesn't turn out too complicated).

@tomasr8

tomasr8 commented Apr 13, 2026

Copy link
Copy Markdown
Member

It's quite related to this change so I'm inclined to fix it in it's PR, if it's ok (and doesn't turn out too complicated).

Yeah the parser assumes what you type is at least somewhat valid, but we can fix it if it's not too complicated. I would consider this a bug so we should backport it, could you make a separate PR for it?

@loic-simon loic-simon mentioned this pull request Apr 15, 2026
Open
@picnixz

picnixz commented Apr 21, 2026

Copy link
Copy Markdown
Member

You can have subpackages so from compression<TAB> could also show from compression.zstd. How should we handle this? Same for math.integer. I do not have a preference but I think we should not complete the import. We should stick to completing members IMO. Or is namespace completion triggered after a period only? (math.<TAB> should complete the namespace but math should complete toimport?)

@loic-simon

loic-simon commented Apr 21, 2026

Copy link
Copy Markdown
Contributor Author

Or is namespace completion triggered after a period only? (math.<TAB> should complete the namespace but math should complete toimport?)

That's the behavior currently proposed by this PR, yes. But I agree that if you're looking for compression.zstd, adding import after from compression<tab> is wrong and can be frustating.

So I guess the question is whether to drop this behavior entirely (eg. only insert import is there is already a space), or to check if there is potential submodules, and insert import only if there isn't.

I'm leaning slightly toward the first option, because I think the second is less obvious and can be complicated to understand as a user, but I'm happy to try and implement it if we think it's worth it!

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

Reviewers

@tomasr8 tomasr8 tomasr8 left review comments

@pablogsal pablogsal Awaiting requested review from pablogsal pablogsal is a code owner

@lysnikolaou lysnikolaou Awaiting requested review from lysnikolaou lysnikolaou is a code owner

@ambv ambv Awaiting requested review from ambv ambv is a code owner

@johnslavik johnslavik Awaiting requested review from johnslavik

Assignees

No one assigned

Labels

awaiting review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@loic-simon @johnslavik @tomasr8 @picnixz