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
Fix completion issue #1160
Fix completion issue #1160
Conversation
So if I understand correctly, IPython is also using |
One little issue remains. Completion works only on prefixes preceded by a whitespace as JQuery Terminal split words at whitespaces. >>> str.isa <tab> <tab>
str.isalnum( str.isalpha( str.isascii(
>>> a = 5 ; str.isa <tab> <tab>
str.isalnum( str.isalpha( str.isascii(
>>> a = 5 ;str.isa <tab> <tab> # no proposition This is known upstream and could be fixed with |
I think having this PR as is is already good. I'll try to review later today. We could always improve it in a follow up PR. |
Thanks, haven't noticed:
Totally agree. |
Great! |
By the way, a major concern with using jedi for autocompletions is that jedi can cause fatal errors #821. It isn't great if requesting a completion causes the Python runtime to segfault. Surely |
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.
Minor comment otherwise LGTM.
Could you please add separate changelog entry for the switch from jedi to rlcompleter in console.html?
# import re | ||
# regex = '|'.join(map(re.escape, self.completer_word_break_characters)) | ||
# start = len(source) - re.search(regex, source[::-1]).start() | ||
start = max(map(source.rfind, self.completer_word_break_characters)) + 1 |
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.
This versions seems faster,
In [10]: %%timeit
...: regex = '|'.join(map(re.escape, completer_word_break_characters)
...: )
...: re.search(regex, source[::-1])
...:
...:
8.74 µs ± 18.7 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)
In [11]: %timeit max(map(source.rfind, completer_word_break_characters))
...: + 1
2.7 µs ± 11.2 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each
particularly given that this benchmark is biased toward regex since regex compilation will be cached.
So I think we can remove the comment. In any case it's µs so it shouldn't matter much.
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.
Great! Thanks for the benchmark!
start = max(map(source.rfind, self.completer_word_break_characters)) + 1 | ||
source = source[start:] | ||
if "." in source: | ||
completions = self._completer.attr_matches(source) # type: ignore |
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.
It's strange that mypy can't find these attributes, type annotations are likely incorrect somewhere in CPython. Not much we can do about it on our side.
Completion implemented in #1155 is not fully working with JQuery terminal in
console.html
. It actually drops every completions that does not start with the prefix it computes (using whitespace splitting). See this comment.One way to fix this is to prepend the prefix to each completion choice but this PR goes another way. CPython has a builtin completion system to work with
readline
calledrlcompleter
which you can use without standalone. This is the completion used in CPython's REPL. This will result in a faster completion experience, without any extra dependency (CPython's core instead of jedi/parso). Moreover, it already computes the completions with the prefix JQuery terminal is looking for.This PR re-implements the completion in
InteractiveConsole
while fixing the issue exposed in this comment.