gh-130472: Integrate fancycompleter with the new repl, to get colored tab completions #130473
Conversation
…'t work because they need to be ported from pytest to unittest
…n exception when evaluated, just color it as None
| namespace = dict(namespace) | ||
| _wrapper.config.readline_completer = RLCompleter(namespace).complete | ||
|
|
||
| if os.getenv('PYTHON_BASIC_COMPLETER'): |
There was a problem hiding this comment.
Should this variable be documented?
There was a problem hiding this comment.
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.
documentation added in b710bce
| # Copyright 2010-2025 Antonio Cuni | ||
| # Daniel Hahler | ||
| # | ||
| # All Rights Reserved |
There was a problem hiding this comment.
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.
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.
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.
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.
The CLA can be signed here: https://cla.python.org/
Co-authored-by: Hugo van Kemenade <[email protected]>
Co-authored-by: Adam Turner <[email protected]> Co-authored-by: Tomas R. <[email protected]>
Co-authored-by: Tomas R. <[email protected]>
Co-authored-by: Hugo van Kemenade <[email protected]>
… into antocuni/fancycompleter
Yes, but it is not documented. You can add an exclamation mark at the start to remove the link. |
Co-authored-by: Stan Ulbrych <[email protected]>
…y this fixes the failures on Android CI
…d color. This is both more robust and more correct
| names += [' '] | ||
| return names | ||
|
|
||
| def colorize_matches(self, names, values): |
There was a problem hiding this comment.
How much time does this add to the autocompletion? I did a quick test with
>>> import sys
>>> sys.[TAB]
Then 86 names are processed with about 10 ms spend in colorize_matches alone. For modules or classes with more attributes the time would be longer. The processing does not seem needed since the response of the autocompletion is [ not unique].
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.
Are there any benchmarks for the full autocompletion or rules-of-thumb how long it should take?
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.
Then 86 names are processed with about 10 ms spend in colorize_matches alone.
this seems way too much.
I ran this super simple benchmarks on my machine:
import sys
import time
from _pyrepl.fancycompleter import Completer
def gendict(n):
return {
f'item{i}': i
for i in range(n)
}
class MyClass:
pass
def main():
obj = MyClass()
comp_col = Completer({'obj': obj}, use_colors=True)
comp_nocol = Completer({'obj': obj}, use_colors=False)
for i in range(0, 10000, 1000):
obj.__dict__ = gendict(i)
a = time.perf_counter()
comp_col.attr_matches('obj.')
b = time.perf_counter()
comp_nocol.attr_matches('obj.')
c = time.perf_counter()
t_col = b - a
t_nocol = c - b
diff_pct = (t_col - t_nocol) / t_nocol * 100
print(f"{i:5d}: col={t_col*1000:.1f}ms nocol={t_nocol*1000:.1f}ms diff={diff_pct:+.2f}%")
main()
and I got this results:
❯ ./python bench_fancycompleter.py
0: col=0.1ms nocol=0.1ms diff=+18.30%
1000: col=0.7ms nocol=0.7ms diff=+5.81%
2000: col=1.6ms nocol=1.4ms diff=+13.75%
3000: col=2.4ms nocol=2.0ms diff=+21.95%
4000: col=2.9ms nocol=2.4ms diff=+19.63%
5000: col=3.8ms nocol=5.0ms diff=-23.60%
6000: col=5.3ms nocol=4.5ms diff=+15.84%
7000: col=5.0ms nocol=4.6ms diff=+9.01%
8000: col=6.9ms nocol=7.6ms diff=-9.75%
9000: col=7.0ms nocol=6.9ms diff=+1.47%
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.
The example minimal example above never reaches colorize_matches because there is a common prefix.
If I replace comp_col.attr_matches('obj.') with comp_col.attr_matches('obj.item') I get this output
0: col=0.1ms nocol=0.0ms diff=+95.88%
2000: col=85.6ms nocol=1.0ms diff=+8489.94%
4000: col=166.8ms nocol=1.9ms diff=+8763.17%
6000: col=286.6ms nocol=3.6ms diff=+7940.23%
8000: col=330.9ms nocol=4.5ms diff=+7297.60%
10000: col=429.9ms nocol=5.1ms diff=+8394.79%
12000: col=509.7ms nocol=6.1ms diff=+8308.30%
14000: col=632.8ms nocol=9.4ms diff=+6627.23%
16000: col=1308.4ms nocol=29.5ms diff=+4331.26%
18000: col=2131.1ms nocol=35.8ms diff=+5855.55%
There was a problem hiding this comment.
oh right, sorry for the silly mistake. Indeed, this is what I get on my machine:
❯ ./python bench_fancycompleter.py
0: col=0.1ms nocol=0.1ms diff=+16.43%
1000: col=9.6ms nocol=1.0ms diff=+826.21%
2000: col=17.8ms nocol=1.3ms diff=+1293.67%
3000: col=26.6ms nocol=1.8ms diff=+1377.77%
4000: col=33.1ms nocol=2.3ms diff=+1335.15%
5000: col=40.8ms nocol=3.7ms diff=+996.66%
6000: col=54.1ms nocol=3.7ms diff=+1368.12%
7000: col=56.0ms nocol=4.1ms diff=+1253.30%
8000: col=63.4ms nocol=6.7ms diff=+851.07%
9000: col=71.9ms nocol=6.0ms diff=+1095.03%
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.
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 gettheme() out of the loop performance is much better:
0: col=0.1ms nocol=0.0ms diff=+114.87%
1000: col=1.8ms nocol=0.5ms diff=+257.26%
2000: col=3.2ms nocol=1.1ms diff=+187.30%
3000: col=3.9ms nocol=1.4ms diff=+182.85%
4000: col=4.9ms nocol=2.0ms diff=+143.63%
5000: col=6.8ms nocol=2.9ms diff=+137.13%
6000: col=8.0ms nocol=3.2ms diff=+152.49%
7000: col=10.0ms nocol=3.5ms diff=+185.24%
8000: col=10.9ms nocol=4.3ms diff=+153.73%
9000: col=12.1ms nocol=4.8ms diff=+149.98%
So I am happy with the current performance of the PR!
Co-authored-by: Pieter Eendebak <[email protected]>
Lib/_colorize.py
Outdated
|
|
||
|
|
||
| @dataclass(frozen=True, kw_only=True) | ||
| class FancyCompleter(ThemeSection): |
There was a problem hiding this comment.
Please keep this class in alphabetical order, like the others.
Same for the fancycompleter below.
I tested it on Windows 10 using the Windows Terminal app. |
| if '(' in expr or ')' in expr: # don't call functions | ||
| return [] | ||
| try: | ||
| thisobject = eval(expr, self.namespace) |
There was a problem hiding this comment.
Could this be the following?
| thisobject = eval(expr, self.namespace) | |
| thisobject = self.namespace.get(expr) |
There was a problem hiding this comment.
no, it doesn't work in case of e.g. a.b.c.<TAB>
There was a problem hiding this comment.
I see. In general I like to avoid using eval in code (because of safety and performance reasons). Not using eval here would add a few lines of code though. Leaving the decision on this to a core dev
| return [] | ||
|
|
||
| if len(names) == 1: | ||
| return [f'{expr}.{names[0]}'] # only option, no coloring. |
There was a problem hiding this comment.
Should we color anyway for consistency?
There was a problem hiding this comment.
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.
Agreed, changing the colors in the prompt is not what we want. Could you update the comment with the reason?
Co-authored-by: Pieter Eendebak <[email protected]>
Lib/_pyrepl/fancycompleter.py
Outdated
| if prefix and prefix != attr: | ||
| return [f'{expr}.{prefix}'] # autocomplete prefix | ||
|
|
||
| names.sort() |
There was a problem hiding this comment.
This does not sort the values. Maybe revert the commit that introduced this if it is too much trouble to fix this.
There was a problem hiding this comment.
yes, pro tip for my future self: don't write code when you are sleepy :).
commit 7d2c790 reverts it, and also adds a comment to explain WHY we need to sort words.
| def _callable_postfix(self, val, word): | ||
| # disable automatic insertion of '(' for global callables | ||
| return word | ||
|
|
There was a problem hiding this comment.
| def _callable_postfix(self, val, word): | |
| # disable automatic insertion of '(' for global callables | |
| return word |
The method does not seem to be used.
There was a problem hiding this comment.
yes, it's used. We are subclassing rlcompleter.Completer and by overriding this method we prevent the insertion of '('.
| values = [] | ||
| for name in names: | ||
| clean_name = name.rstrip(': ') | ||
| if clean_name in keyword.kwlist: |
There was a problem hiding this comment.
| if clean_name in keyword.kwlist: | |
| if keyword.iskeyword(clean_name): |
| words = set(dir(thisobject)) - {'__builtins__'} | ||
|
|
||
| if hasattr(thisobject, '__class__'): | ||
| words.add('__class__') |
There was a problem hiding this comment.
If I remove this line, tests still pass. Why is this needed?
There was a problem hiding this comment.
this is a valid question, and I admit I don't know the answer.
The code was originally introduced by this PR in fancycompleter, which merged some logic from CPython's rlcompleter.py:
pdbpp/fancycompleter#17
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.
There was a problem hiding this comment.
It was using words.discard, but then I committed this suggestion by @AA-Turner
#130473 (comment)
I'm happy with both 🤷♂️
There was a problem hiding this comment.
If we can remove the line in fancycompleter and nothing breaks, we may as well do so.
There was a problem hiding this comment.
Both the words.add('__class__') and words.discard("__builtins__") can be removed in rlcompleter.py without tests breaking (locally for me). The words.add('__class__') seems rather safe to me (if __class__ is an attribute of thisobject, it should be in dir(thisobject)).
Removing words.discard("__builtins__") is also fine with me, but I have no knowledge of the history here. The commit states
Do not expose builtins name as a completion; this is an implementation detail that confuses too many people. Based on discussion in python-dev.
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 __spec__, __package__, __build_class__ and a few more), so why would __builtins__ be special. Maybe in the python version back then __builtins__ was more prominent? Again, it is so long ago I cannot install a python version from that time on my system.
There was a problem hiding this comment.
Note: if we decide to remove these, I would do it in a separate PR.
| except Exception: | ||
| return [] | ||
|
|
||
| # get the content of the object, except __builtins__ |
There was a problem hiding this comment.
Why do we have to remove __builtins__? If I remove this part the unit tests still pass.
There was a problem hiding this comment.
same answer as above, it's using the same logic as rlcompleter
…vance, let's add a comment to explain why
As the title says, this is (WIP) to integrate fancycompleter into the new REPL.