-
Notifications
You must be signed in to change notification settings - Fork 137
Code completion improvements #237
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
Conversation
The helper was originally introduced to make the detection of a word at a given point more accurate (this is used for completion, hovering, etc) and fixes some issues with the tree sitter grammar. It turns out this is really not needed and introduces a bunch of issues. Related to #192
Codecov Report
@@ Coverage Diff @@ ## master #237 +/- ## ========================================== - Coverage 73.92% 73.57% -0.36% ========================================== Files 18 18 Lines 560 545 -15 Branches 90 87 -3 ========================================== - Hits 414 401 -13 + Misses 125 124 -1 + Partials 21 20 -1
Continue to review full report at Codecov.
|
62b101d to
01a692c
Compare
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.
is this an optimization to avoid creating the massive allCompletions array below?
If so, this if statement only returns when the user type:
echo $
echo ${
echo "$
echo "${
and if the user types anything after that, the value of shouldCompleteOnVariables will be false.
Is this desired behavior, or do we want shouldCompleteOnVariables to be true when the user has something like:
start="some variable" # user begins to type 'st': # and hopes to autocomplete 'start' echo ${st # shouldCompleteOnVariables is false though
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.
Thanks for reading through this carefully.
is this an optimization to avoid creating the massive allCompletions array below?
This is actually not an optimization. The reason for the early exit is that we only want to suggest parameters/variables here if the word that we complete on is $ or ${.
and if the user types anything after that, the value of shouldCompleteOnVariables will be false.
From my experience, this is not how language server protocol clients work. They get completion for a word (e.g. $) once, meaning they do not continuously call the onCompletion while the user types. This makes our life a bit easier here, as we do not need to adjust while the user types out a word.
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 variable is used to test if:
const shouldCompleteOnVariables = word ? PARAMETER_EXPANSION_PREFIXES.has(word) : false
This check sets shouldCompleteOnVariables to true only for the first two, and only if the user hasnt typed anything else. I've done some tests, and I can't find a case where ${# gets detected.
The PR looks very nice! I tested it out and it works well for most cases that I've tried.
I made a few comments about the variables that it shows for completion, and how basically, it will show you variables if you type: $ or ${ but if you start typing anything after that, it will show you all variables, builtins, reserved words, etc.
I think this issue is something with tree sitter like you said in #236
When you type ${a then the const word = this.getWordAtPoint word becomes a, instead of ${a. but maybe this is desired?
In either case, I think it looks good. 👍
Previously this was skipped as the parser logic was broken.
I made a few comments about the variables that it shows for completion, and how basically, it will show you variables if you type: $ or ${ but if you start typing anything after that, it will show you all variables, builtins, reserved words, etc.
I don't think this is the case for most use cases. As I commented above, the client of an LSP will only call once a word is started (e.g $), and not while the user types (e.g $MY_VARIA).
But if you go to an existing line with an unfinished word (e.g $MY_VARIA) and auto completes, then we do not currently know that we are in a parameter expansion, hence we will autocomplete on not only parameters (MY_VARIABLE), but also functions, built-ins, etc. This should be a future extension, where we based on the grammar, would see if the current word inside a parameter expansion.
I documented this in e6f759b
When you type ${a then the const word = this.getWordAtPoint word becomes a, instead of ${a. but maybe this is desired?
Yes, form the parser's side, a is a word inside the parameter expansion. So this is expected behavior.
@rcjsuen @nikita-skobov thanks for the review. Much appreciated! 👏
Language server changelog: 1.16.1 Fix brace expansion bug (bash-lsp/bash-language-server#240) Do not crash if bash is not installed (bash-lsp/bash-language-server#242) 1.16.0 Improved completion handler for parameter expansions (bash-lsp/bash-language-server#237) 1.15.0 Use comments above symbols for documentation (bash-lsp/bash-language-server#234, bash-lsp/bash-language-server#235) 1.14.0 onHover and onCompletion documentation improvements (bash-lsp/bash-language-server#230) support 0/1 as values for HIGHLIGHT_PARSING_ERRORS (bash-lsp/bash-language-server#231) 1.13.1 Gracefully handle glob failures (bash-lsp/bash-language-server#224, bash-lsp/bash-language-server#226) Maintenance (bash-lsp/bash-language-server#222, bash-lsp/bash-language-server#225) Signed-off-by: Alexander Kurtakov <akurtako@redhat.com>
Language server changelog: 1.16.1 Fix brace expansion bug (bash-lsp/bash-language-server#240) Do not crash if bash is not installed (bash-lsp/bash-language-server#242) 1.16.0 Improved completion handler for parameter expansions (bash-lsp/bash-language-server#237) 1.15.0 Use comments above symbols for documentation (bash-lsp/bash-language-server#234, bash-lsp/bash-language-server#235) 1.14.0 onHover and onCompletion documentation improvements (bash-lsp/bash-language-server#230) support 0/1 as values for HIGHLIGHT_PARSING_ERRORS (bash-lsp/bash-language-server#231) 1.13.1 Gracefully handle glob failures (bash-lsp/bash-language-server#224, bash-lsp/bash-language-server#226) Maintenance (bash-lsp/bash-language-server#222, bash-lsp/bash-language-server#225) Signed-off-by: Alexander Kurtakov <akurtako@redhat.com>
Uh oh!
There was an error while loading. Please reload this page.
Fixes #236
This PR enables code completion improvements:
else(we had a bug in the handling around tree-sitter)Screenshots (before and after)
Screenshot 2020年05月20日 10 42 28 Screenshot 2020年05月20日 10 42 34
Screenshot 2020年05月20日 10 43 07 Screenshot 2020年05月20日 10 42 42