-
Notifications
You must be signed in to change notification settings - Fork 29
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
feat: intellisense > prioritize column items over others #465
feat: intellisense > prioritize column items over others #465
Conversation
I don't think I understand. Doesn't the cache cause different suggestions when it's hit?
This hack makes me a bit uncomfortable... given where things are now, I'd rather push back on requirements that require us to do things like this. If we were super on-top-of the current language server code adding a hack like this might be less risky, but as it is, I'd rather not add unnecessary complexity/risk. That all said, I'm not really familiar enough with this code to fully understand how risky this really is. If you want to go over it more slowly I'd be happy to hop on a call. |
const isIncluded = word?.includes(lastWord); | ||
const shouldGetItems = !lastWord || !word || !isIncluded; | ||
if (shouldGetItems) { | ||
completionList = await getFromLanguageService(resource, position); |
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.
If we were on top of things I'd expect to see something after the wait point to check if we're still active or not.
I know our language server doesn't currently have much in the way of cancelation support, so I'm not expecting this PR to fix it.
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.
Not sure I understand
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.
Actually, I am sure I don't :)
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.
For example, if we leave the page completion items are being fetched, we should stop progress.
Right now it's pretty easy to create exceptions by quickly closing the page after doing something to query text when the language server is active.
const mockGetFromLanguageService: jest.MockedFunction<GetFromLanguageService> = jest.fn(); | ||
const resource = {} as monaco.Uri; | ||
const position = {} as ls.Position; |
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.
Nit: Do these need to be fake? Could we pass things that type-check to make the test more durable?
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.
Fakes are test doubles that implement business rules. In contrast, dummies are objects that are passed around but never actually used. They typically fill parameter lists when no operations are needed on that object. This scenario is ideal for using dummies. Dummies help simplify the test setup by acting as placeholders for required objects that are not the focus of the test. In this case, the focus of the test is whether the languageService was called or not.
} | ||
|
||
lastWord = 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.
} | |
lastWord = word; | |
lastWord = word; | |
} | |
If we update the last work when the completion items haven't changed, don't we make it possible to not get completion items even when the new word make sense for the original completionList
?
For example, if we call this with these words: "asdf", "a", "df". "df" won't hit the cache because "a" proceeded it, but would have if we omit "a".
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.
Let's simulate what happens when we call it with these words in this order (I am mimicking it character by character as the user would type it on their keyboard. Copy-pasting doesn't trigger the CompletionAdapter at all):
"a" - miss - calls LS (language service)
"as" - hit - doesn't call LS
"asd" - hit
"asdf" - hit
"asd" - CompletionAdapter isn't triggered when deleting
"as" - not triggered
"a" - not triggered
"" - not triggered
"a" - miss as lastWord is "asdf" and it is not contained in "a"
"" - not triggered
"d" - miss as "a" is not contained in "d"
"df" - hit as "d" is included in "df"
So, if I understood you correctly, I think we are okay in this case. What do you think?
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.
There's a few things I'm pretty sure that will happen that I think are pretty weird:
"as" - hit - doesn't call LS
"asd" - hit
"asdf" - hit
Are we sure it makes sense to use cached results with these? I'd expect "asdf" to return pretty different results from "a"
"a" - miss as lastWord is "asdf" and it is not contained in "a"
Even though we updated the last word, the last completion result was "a". If we only updated the word when we update this completion result, this would be a cache hit.
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.
Are you expecting "asdf" to return completely different results or just a subset of the results returned from "a"? The Monaco editor filters the results, so as long as the results from "asdf" are a subset, we're fine. Previously, before setting incomplete: true, the Monaco editor would call provideCompletionItems only on the first character (and also on the fourth). I attempted to replicate this logic here (except for the fourth character as it produced strange results).
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.
How feasible would it be to initialize an annual language server instance, and write tests against 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.
An annual language server? I am not sure I understand.
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.
Sorry, meant to type “actual”
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.
In this case, it is unnecessary since the test is only checking if the language service is called, not the specifics of how it is called.
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 guess I'm a little worried the tests are a little too unit-y. It would be nice to have tests that run through full scenarios like you describe here [1].
[1] #465 (comment)
I'd try and figure out how we can snapshot completion results based on a list of user inputs, so we can see how the completion suggestions change when we update the code.
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.
You are absolutely correct, these are unit tests. I love unit tests because they run quickly and test pure logic, which is exactly what I need in this case. I completely agree that having a full scenario test is also crucial, and we do have one. I began the task by writing an integration acceptance test, which you can see here.
@maxburs the hack here is defiantly not ideal, but I think it's worth the risk:
|
This is a small workaround to prevent the Monaco editor from re-arranging the IntelliSense completion items based on the user's search term.
Firstly, I trigger provideCompletionItems on every key press by setting incomplete=true so I can change the filterText dynamically. To avoid calling the language server on each key press, I implemented a caching mechanism that prevents unnecessary calls.
Additionally, I prepend the user search term to column type items only when the search term is present in the filter text.
Since the Monaco editor re-arranges items where the search term appears first in the filterText, this ensures that the column items are brought up in the IntelliSense order.
** One thing missing in the implementation is that currently, the Monaco editor filters items using a fuzzy search, and I am not applying fuzzy matching to column names. We need to make a decision here: either implement fuzzy matching for column names and search terms as well, or remove the fuzzy search from the Monaco editor as Michal originally wanted.