gh-69605: PyREPL: insert "import" after "from foo <tab>"#148445
gh-69605: PyREPL: insert "import" after "from foo <tab>"#148445loic-simon wants to merge 1 commit intopython:mainfrom
Conversation
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)
loic-simon
requested review from
ambv,
lysnikolaou and
pablogsal
as code owners
tomasr8
left a comment
tomasr8
left a comment
There was a problem hiding this 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!
| ("from importlib.res\t\n", "from importlib.resources"), | ||
| ("from importlib.\t\tres\t\n", "from importlib.resources"), | ||
| ("from importlib.resources.ab\t\n", "from importlib.resources.abc"), | ||
| ("from impo\t\n", "from importlib "), |
There was a problem hiding this comment.
I'm not sure whether we should change this, what do you think? importlib has submodules so I think we should not add the final space, in case you want to type something like from importlib.foo ...
There was a problem hiding this comment.
Hmm yes, I didn't thought of that! But that applies to a lot of modules (including my example of from mat import <tab><tab>)...
I think having a different behaviour if the module has submodules or not would be both obscure and hard to implement.
So I see two three paths forward:
- abandon the space insertion idea, and stick to "insert import" only
- keep it anyway (for all imports), but I think that can be quite frustating if you want to import from a submodule indeed
- don't insert space, but add a new special case: when the text to complete is an exact match, insert
import.
eg. to come back to my table:
| Input | Current | Proposed |
|---|---|---|
from math <tab> |
from math |
from math import |
from mat<tab> |
from math |
from math (no more extra space) |
from math<tab> |
from math |
from math import (insert space + import) |
That allow from mat<tab><tab> to still work, and should be quite straightforward, so at a glance I quite like it!
| - `import foo` -> Result(from_name=None, name='foo') | ||
| - `import foo.` -> Result(from_name=None, name='foo.') | ||
| - `from foo` -> Result(from_name='foo', name=None) | ||
| - `from foo ` -> Result(from_name='foo', name=None, end_space=True) |
|
I had the same idea a few days ago. Nice! I'll review soon. |
|
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). |
3 participants
Two import completion improvements:
from math <tab>from mathfrom math importfrom mat<tab>from mathfrom math(extra space)The first one should be pretty uncontroversial, since
importis the only valid syntax here.The second only change the behavior when a single match is found:
from cont<tab>still insertsfrom context(common prefix tocontextlibandcontextvar)from foo<tab>still inserts nothing, since we have no matchIt allows
from mat<tab><tab>to insertfrom math import, which I find really smooth and pleasant to use!Implementation notes: I settled to extend
ImportParser.parseto also return aspace_endboolean, and use that inModuleCompleter.completeto return animportsuggestion / 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?)