- Notifications
You must be signed in to change notification settings - Fork664
SimplifyfilterText computation + fixes#1414
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 ourterms of service andprivacy 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?
Conversation
| iffilterText=="" { | ||
| filterText=getFilterText(file,position,insertText,name,isMemberCompletion,isSnippet,wordStart) | ||
| } | ||
| ifisMemberCompletion&&!isSnippet { |
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 code here was removed because it mostly overlapped withgetFilterText above.
| m(){ | ||
| this.; |
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 results from fixing the scanner bug.
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.
Seems okay, just wanted to clarify a few things before approving.
Uh oh!
There was an error while loading.Please reload this page.
| @@ -2127,41 +2143,45 @@ func getFilterText( | |||
| // In which case we want to insert a bracket accessor but should use `.abc` as the filter text instead of | |||
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 is sort of an aside, but I don't understand why conceptually the "access operator" (the. or?.) is part of the filter text at all.
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 client will rank the completion items by comparing the filter text with some suffix of the existing test at the completions position. The suffix is usually the word touching the position, but may be a custom range when the item specifies a text edit.
Given that, we want to use the filter text to avoid the client overly de-ranking or omitting completions that have to modify some of the text that the user already typed.
For instance, if we have this:
declareconstobj:{"ab c":string};obj.a|
The completion item forab c will need to insert["ab c"], and it will specify a range in the text edit covering.a, but we want it to be ranked by the client the same as a regular property should, so the client should compare.a (text at current position that the item will replace) with.ab c, instead of["ab c"], because["ab c"] doesn't really match.a.
The access operator is there because in the situations where we use it in the filter text, it is part of the text that will be replaced if you accept the completion item, so it is part of what will be compared with the filter text.
That said, it's possible there are some cases where we currently set the filter text that could be optimized away.
| } | ||
| s.token=ast.KindPrivateIdentifier |
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.
Seems fine, but why did we need this? Or, why is Strada able to get away without this?
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 new code does what Strada does. The code before this PR seems like a porting bug.
Uh oh!
There was an error while loading.Please reload this page.
This PR simplifies the computation of a completion item's
filterTextwhich was ported from the VSCode TS extension. There were originally two pieces of code wherefilterTextwas computed, and I think one of those was mostly unnecessary, so I removed it and simplified the rest of the code.The code I removed used to set
filterTextfor all dot completions, which wasn't needed, so the Corsa tests have been updated to reflect that, and a bunch of tests ported from Strada now pass as well because we no longer setfilterTextin most cases.I also added tests for all the cases where I think
filterTextis necessary, since this piece of code is new and so not covered by Strada tests.This PR also fixes two porting bugs, one in the parser and one bug in the scanner, and some minor completion bugs.