gh-130472: Use fancycompleter in import completions#148188
gh-130472: Use fancycompleter in import completions#148188tomasr8 wants to merge 7 commits intopython:mainfrom
Conversation
- Make module completer return both names and values (dummy `sys` module in case of module completions) - Colorize completions using `colorize_matches` from FancyCompleter
loic-simon
left a comment
loic-simon
left a comment
There was a problem hiding this comment.
Nice!! Here's a quick first pass, I'll play around with it in local later 😄
| def get_completions(self, line: str) -> tuple[list[str], CompletionAction | None] | None: | ||
| def get_completions(self, line: str) -> tuple[list[str], list[Any], CompletionAction | None] | None: |
There was a problem hiding this comment.
I would've find it more natural to return a dict[str, Any] instead of two parallel lists, but I see that's what fancycompleter does, so let be it!
But I wonder if we haven't passed the point where a namedtuple would be more readeable / maintainable? I hesitated when adding completion action, something like
class _GetCompletionsReturn(NamedTuple):
names: list[str]
values: list[Any]
action: CompletionAction | Nonethe docstring could use an update either way 😄
| all_values = [attr_map.get(n) if n not in submodule_set else sys | ||
| for n in all_names] |
There was a problem hiding this comment.
If a name is both a submodule and an attribute, the attribute takes precedence at runtime, so I believe this should be
| all_values = [attr_map.get(n) if n not in submodule_set else sys | |
| for n in all_names] | |
| all_values = [attr_map[n] if n in attr_map else sys for n in all_names] |
| readline_completer: Completer | None = None | ||
| completer_delims: frozenset[str] = frozenset(" \t\n`~!@#$%^&*()-=+[{]}\\|;:'\",<>/?") | ||
| module_completer: ModuleCompleter = field(default_factory=make_default_module_completer) | ||
| colorize_completions: Callable[[list[str], list[object]], list[str]] | None = None |
There was a problem hiding this comment.
Do you have a rationale for using list[object] here but list[Any] in _module_completer? I think it should be object anywhere (cf. mypy doc)
| f"{module_color}importlib.metadata{R}", | ||
| ]) | ||
| self.assertIsNone(action) | ||
|
|
There was a problem hiding this comment.
maybe a test with a name both an attribute + a submodule with the same name would be great, to be sure everything combines well?
2 participants
Now that #130473 has been merged, let's use it for import completions as well.
This is my first stab at this. First, I did a bit of refactoring and extracted the getattr and colorize logic from
FancyCompleterinto reusable functions since I needed those for module completions. I also modifiedModuleCompleterto return attribute values in addition to names (for modules, it just returns thesysmodule as a dummy value, this is just to get the color right). Then I just needed to extendget_module_completionsto do the colorization.@loic-simon would love if you have some spare time to have a look :)