gh-69605: PyREPL: insert "import" after "from foo <tab>" - #148445
gh-69605: PyREPL: insert "import" after "from foo <tab>"#148445loic-simon wants to merge 3 commits into
Conversation
Sorry, something went wrong.
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)
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!
Sorry, something went wrong.
| ("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 ...
Sorry, something went wrong.
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!
Sorry, something went wrong.
There was a problem hiding this comment.
I think having a different behaviour if the module has submodules or not would be both obscure and hard to implement.
Hard probably, but UX-wise I think it'd be nice if it added the space for modules that don't have submodules. It would save you one keystroke and immediately signal that there are no submodules to import from.
Sorry, something went wrong.
There was a problem hiding this comment.
Okay, I implemented the point 3 above, I tend to think it achieves the same smooth behavior (double tab to insert import) and is more simple to reason about.
But I'm eager to hear what you think, and happy to try and implement the "no submodules case" solution if we think it's better!
Sorry, something went wrong.
|
I had the same idea a few days ago. Nice! I'll review soon. |
Sorry, something went wrong.
|
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). |
Sorry, something went wrong.
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? |
Sorry, something went wrong.
|
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?) |
Sorry, something went wrong.
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! |
Sorry, something went wrong.
|
This PR is stale because it has been open for 90 days with no activity. |
Sorry, something went wrong.
Two import completion improvements:
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:
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?)