-
-
Notifications
You must be signed in to change notification settings - Fork 33k
gh-130472: Integrate fancycompleter with the new repl, to get colored tab completions #130473
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 46 commits
4eb226b
c5dfc85
b561a2e
5f56673
40563f2
88181da
77c578a
9069419
bdbe022
0ea7c49
6ddfa61
30abd71
983824a
acbafe4
5546b94
327648f
44549b9
c9c97f7
c61bab6
1de48b6
157f4b4
722ec8a
4554a9d
e2294ec
4532850
1c3dd97
7089323
926c1a3
b6385e9
46db5d7
c3ea737
433ae06
38e8a08
4c3ad9c
053da64
2ee47bc
c3c663e
b710bce
13a2698
72f30d9
b1f86ae
af1d74e
c52fd7a
fdca77e
7f9d09c
3a6bcd3
743e661
2ebf50e
bdf54ed
6a5bcfe
7d2c790
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,175 @@ | ||||||||||||||
# Copyright 2010-2025 Antonio Cuni | ||||||||||||||
# Daniel Hahler | ||||||||||||||
# | ||||||||||||||
# All Rights Reserved | ||||||||||||||
"""Colorful tab completion for Python prompt""" | ||||||||||||||
from _colorize import ANSIColors, get_colors, get_theme | ||||||||||||||
import rlcompleter | ||||||||||||||
import types | ||||||||||||||
import keyword | ||||||||||||||
|
||||||||||||||
class Completer(rlcompleter.Completer): | ||||||||||||||
""" | ||||||||||||||
When doing someting like a.b.<tab>, display only the attributes of | ||||||||||||||
b instead of the full a.b.attr string. | ||||||||||||||
Optionally, display the various completions in different colors | ||||||||||||||
depending on the type. | ||||||||||||||
""" | ||||||||||||||
def __init__( | ||||||||||||||
self, | ||||||||||||||
namespace=None, | ||||||||||||||
*, | ||||||||||||||
use_colors='auto', | ||||||||||||||
consider_getitems=True, | ||||||||||||||
): | ||||||||||||||
from _pyrepl import readline | ||||||||||||||
rlcompleter.Completer.__init__(self, namespace) | ||||||||||||||
if use_colors == 'auto': | ||||||||||||||
# use colors only if we can | ||||||||||||||
use_colors = get_colors().RED != "" | ||||||||||||||
self.use_colors = use_colors | ||||||||||||||
self.consider_getitems = consider_getitems | ||||||||||||||
|
||||||||||||||
if self.use_colors: | ||||||||||||||
readline.parse_and_bind('set dont-escape-ctrl-chars on') | ||||||||||||||
if self.consider_getitems: | ||||||||||||||
delims = readline.get_completer_delims() | ||||||||||||||
delims = delims.replace('[', '') | ||||||||||||||
delims = delims.replace(']', '') | ||||||||||||||
readline.set_completer_delims(delims) | ||||||||||||||
|
||||||||||||||
def complete(self, text, state): | ||||||||||||||
# if you press <tab> at the beginning of a line, insert an actual | ||||||||||||||
# \t. Else, trigger completion. | ||||||||||||||
if text == "": | ||||||||||||||
return ('\t', None)[state] | ||||||||||||||
else: | ||||||||||||||
return rlcompleter.Completer.complete(self, text, state) | ||||||||||||||
|
||||||||||||||
def _callable_postfix(self, val, word): | ||||||||||||||
# disable automatic insertion of '(' for global callables | ||||||||||||||
return word | ||||||||||||||
|
||||||||||||||
Comment on lines
+54
to
+57
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
The method does not seem to be used. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, it's used. We are subclassing |
||||||||||||||
def global_matches(self, text): | ||||||||||||||
names = rlcompleter.Completer.global_matches(self, text) | ||||||||||||||
prefix = commonprefix(names) | ||||||||||||||
if prefix and prefix != text: | ||||||||||||||
return [prefix] | ||||||||||||||
|
||||||||||||||
names.sort() | ||||||||||||||
values = [] | ||||||||||||||
for name in names: | ||||||||||||||
clean_name = name.rstrip(': ') | ||||||||||||||
if clean_name in keyword.kwlist: | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||
values.append(None) | ||||||||||||||
else: | ||||||||||||||
try: | ||||||||||||||
values.append(eval(name, self.namespace)) | ||||||||||||||
except Exception as exc: | ||||||||||||||
values.append(None) | ||||||||||||||
if self.use_colors and names: | ||||||||||||||
return self.colorize_matches(names, values) | ||||||||||||||
return names | ||||||||||||||
|
||||||||||||||
def attr_matches(self, text): | ||||||||||||||
expr, attr = text.rsplit('.', 1) | ||||||||||||||
if '(' in expr or ')' in expr: # don't call functions | ||||||||||||||
return [] | ||||||||||||||
try: | ||||||||||||||
thisobject = eval(expr, self.namespace) | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could this be the following?
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no, it doesn't work in case of e.g. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see. In general I like to avoid using |
||||||||||||||
except Exception: | ||||||||||||||
return [] | ||||||||||||||
|
||||||||||||||
# get the content of the object, except __builtins__ | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we have to remove There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same answer as above, it's using the same logic as rlcompleter |
||||||||||||||
words = set(dir(thisobject)) - {'__builtins__'} | ||||||||||||||
|
||||||||||||||
if hasattr(thisobject, '__class__'): | ||||||||||||||
words.add('__class__') | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I remove this line, tests still pass. Why is this needed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is a valid question, and I admit I don't know the answer. The current rlcompleter.py contains exactly the same lines: Lines 162 to 167 in 7d2c790
So, I think it's a good idea to keep them in sync. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was using I'm happy with both 🤷♂️ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we can remove the line in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Both the Removing
The commit is so old, I could not find the python-dev archives containing the rationale. On modern Python there are a few more dunder names the might be confusing (e.g. in the global namespace we have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note: if we decide to remove these, I would do it in a separate PR. |
||||||||||||||
words.update(rlcompleter.get_class_members(thisobject.__class__)) | ||||||||||||||
names = [] | ||||||||||||||
values = [] | ||||||||||||||
n = len(attr) | ||||||||||||||
if attr == '': | ||||||||||||||
noprefix = '_' | ||||||||||||||
elif attr == '_': | ||||||||||||||
noprefix = '__' | ||||||||||||||
else: | ||||||||||||||
noprefix = None | ||||||||||||||
words = sorted(words) | ||||||||||||||
eendebakpt marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||
while True: | ||||||||||||||
for word in words: | ||||||||||||||
if ( | ||||||||||||||
word[:n] == attr | ||||||||||||||
and not (noprefix and word[:n+1] == noprefix) | ||||||||||||||
): | ||||||||||||||
try: | ||||||||||||||
val = getattr(thisobject, word) | ||||||||||||||
except Exception: | ||||||||||||||
val = None # Include even if attribute not set | ||||||||||||||
|
||||||||||||||
names.append(word) | ||||||||||||||
values.append(val) | ||||||||||||||
if names or not noprefix: | ||||||||||||||
break | ||||||||||||||
if noprefix == '_': | ||||||||||||||
noprefix = '__' | ||||||||||||||
else: | ||||||||||||||
noprefix = None | ||||||||||||||
|
||||||||||||||
if not names: | ||||||||||||||
return [] | ||||||||||||||
|
||||||||||||||
if len(names) == 1: | ||||||||||||||
return [f'{expr}.{names[0]}'] # only option, no coloring. | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we color anyway for consistency? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no. When you return a single completion, readline inserts it in the prompt. If we return colors here, we would be ANSI codes in the prompt, which is surely not what you want There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed, changing the colors in the prompt is not what we want. Could you update the comment with the reason? |
||||||||||||||
|
||||||||||||||
prefix = commonprefix(names) | ||||||||||||||
if prefix and prefix != attr: | ||||||||||||||
return [f'{expr}.{prefix}'] # autocomplete prefix | ||||||||||||||
|
||||||||||||||
if self.use_colors: | ||||||||||||||
return self.colorize_matches(names, values) | ||||||||||||||
|
||||||||||||||
if prefix: | ||||||||||||||
names += [' '] | ||||||||||||||
antocuni marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||
return names | ||||||||||||||
|
||||||||||||||
def colorize_matches(self, names, values): | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How much time does this add to the autocompletion? I did a quick test with
Then 86 names are processed with about 10 ms spend in Are there any benchmarks for the full autocompletion or rules-of-thumb how long it should take? (maybe these questions are better suited for a separate issue or followup PR) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I don't think there are any but I think that as long as is not slow enough even if the module is too big I don't think it matters. We can always also deactivate if there are too many elements in the module There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
this seems way too much.
and I got this results:
so, on my machine it spends 7ms to process 9000 names, and the difference between colors and no colors is basically noise. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The example minimal example above never reaches
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh right, sorry for the silly mistake. Indeed, this is what I get on my machine:
I'm not an UI expert at all, but I think that anything <100ms is perceived as "instantaneous" by the user, so unless we have modules with >9000 we should be safe 😅. Jokes apart, I see that on your machine things are slower, but I still think that with "normal" number of completions the delay won't be noticeable (and, anecdotally, I never noticed in all these years). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Anecdotally, the timings are from my fast machine, and I have noticed the new pyrepl being slower (not specifically this PR though). With the commit moving the
So I am happy with the current performance of the PR! |
||||||||||||||
matches = [self.color_for_obj(i, name, obj) | ||||||||||||||
for i, (name, obj) | ||||||||||||||
in enumerate(zip(names, values))] | ||||||||||||||
# We add a space at the end to prevent the automatic completion of the | ||||||||||||||
# common prefix, which is the ANSI escape sequence. | ||||||||||||||
matches.append(' ') | ||||||||||||||
return matches | ||||||||||||||
|
||||||||||||||
def color_for_obj(self, i, name, value): | ||||||||||||||
t = type(value) | ||||||||||||||
color = self.color_by_type(t) | ||||||||||||||
# hack: prepend an (increasing) fake escape sequence, | ||||||||||||||
# so that readline can sort the matches correctly. | ||||||||||||||
N = f"\x1b[{i:03d};00m" | ||||||||||||||
return f"{N}{color}{name}{ANSIColors.RESET}" | ||||||||||||||
|
||||||||||||||
def color_by_type(self, t): | ||||||||||||||
theme = get_theme() | ||||||||||||||
eendebakpt marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||
typename = t.__name__ | ||||||||||||||
# this is needed e.g. to turn method-wrapper into method_wrapper, | ||||||||||||||
# because if we want _colorize.FancyCompleter to be "dataclassable" | ||||||||||||||
# our keys need to be valid identifiers. | ||||||||||||||
typename = typename.replace('-', '_').replace('.', '_') | ||||||||||||||
return getattr(theme.fancycompleter, typename, ANSIColors.RESET) | ||||||||||||||
|
||||||||||||||
|
||||||||||||||
def commonprefix(names, base=''): | ||||||||||||||
eendebakpt marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||
"""Return the common prefix of all 'names' starting with 'base'""" | ||||||||||||||
if base: | ||||||||||||||
names = [x for x in names if x.startswith(base)] | ||||||||||||||
if not names: | ||||||||||||||
return '' | ||||||||||||||
s1 = min(names) | ||||||||||||||
s2 = max(names) | ||||||||||||||
for i, c in enumerate(s1): | ||||||||||||||
if c != s2[i]: | ||||||||||||||
return s1[:i] | ||||||||||||||
return s1 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -40,6 +40,7 @@ | |
from .completing_reader import CompletingReader | ||
from .console import Console as ConsoleType | ||
from ._module_completer import ModuleCompleter, make_default_module_completer | ||
from .fancycompleter import Completer as FancyCompleter | ||
|
||
Console: type[ConsoleType] | ||
_error: tuple[type[Exception], ...] | type[Exception] | ||
|
@@ -608,8 +609,13 @@ def _setup(namespace: Mapping[str, Any]) -> None: | |
# set up namespace in rlcompleter, which requires it to be a bona fide dict | ||
if not isinstance(namespace, dict): | ||
namespace = dict(namespace) | ||
|
||
if os.getenv('PYTHON_BASIC_COMPLETER'): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this variable be documented? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, totally! I think it's worth discussing what is the best way to disable fancycompleter, if anybody has other ideas I'm happy to hear There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. documentation added in b710bce |
||
Completer = RLCompleter | ||
else: | ||
Completer = FancyCompleter | ||
_wrapper.config.module_completer = ModuleCompleter(namespace) | ||
_wrapper.config.readline_completer = RLCompleter(namespace).complete | ||
_wrapper.config.readline_completer = Completer(namespace).complete | ||
|
||
# this is not really what readline.c does. Better than nothing I guess | ||
import builtins | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this have a licence grant (e.g. BSD-3-Clause)? If so, we should add it here and to the included software licences section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another option, which might be slightly nicer, is for the contributors of the relevant bits of
fancycompleter
to sign the CLA and have this work relicenced under the PSF licence. This seems to include @blueyed, @theY4Kman, @singingwolfboy, and @slinkp.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to do so
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only made this one obsolete commit to fancycompleter 13 years ago, but i'll sign if it helps :) How do I do that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not expert in licensing issue. I'm happy to do/sign whatever is needed to land the PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CLA can be signed here: https://cla.python.org/