Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Send completion request to multiple sessions #1582
Send completion request to multiple sessions #1582
Changes from 25 commits
cfecfdc
ebd124a
20dd3e7
633d9ca
66d0666
6c6837d
9074230
08c4f3d
f8dc0ea
c1d1948
14c0e41
c0a93c7
64e3e29
adda5e4
4432bc9
f4ba032
1f608ec
0183d43
d19284c
8f0d43e
fa61e6a
369c895
d2deb69
04345e2
a6f7189
4281463
60b8869
01a9813
934db7a
64a60e8
d6897e7
1a02f82
4f0d591
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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'm thinking it would be better to clear the list from
_on_all_settled
. In theory, with slow-running requests, it could happen that some stale completions would sneak into the latest request 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.
To add on this, here is how the code currently behaves.
If a language server has
isIncomplete
completions and we:In that case an index error will be thrown.
In that case moving the clearing logic to
_on_all_settled
would solve the problem.But then we would clear the list and then immediately populate it: (why would we clear the list before assigning a new completion, when we can just assign the new completions)
https://github.com/sublimelsp/LSP/pull/1582/files#diff-982c110867d9809c978a5597e4da66ce7538b327675a66b8f27214413b7c0ec1R675
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.
Good point. I thought that with incomplete completions the popup will close on typing the next character but it doesn't.
But on the other hand, with the current code, there could be another issue:
With
isIncomplete
completions, the user types "a" and then "b". Both requests are started but are slow to respond so when they both resolve, they both append items to theLspResolveDocsCommand.completions
list, and triggering documentation could then fail in some cases.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 a couple of approaches that came to my mind:
a) never clear the completions. (currently we only clear them when we trigger the AC)
LspResolveDocsCommand.completions = []
b) pass the completion item instead of the index: (feels like the a right approach, but performance might be affected)
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 would choose
a)
because it is simpler and not that different from the current behaviuor.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 can this work if we never clear them?
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.
We assign the response to
LspResolveDocsCommand.completions
(keep in mind that I no longer useLspResolveDocsCommand.completions.entend(response_items)
like I used to. Instead I just use=
as this will prevent the list from growing to infinity)01a9813#diff-982c110867d9809c978a5597e4da66ce7538b327675a66b8f27214413b7c0ec1R675
When we want to resolve the completion item,
we will look up the completion item in the
LspResolveDocsCommand.completions
01a9813#diff-1e31f37d39e534dca2dca7fd81d29d7e5a039a4b55a5765fdb4292c7a87760aaR21
That is it.
There is no need to clear the
LspResolveDocsCommand.completions = {}
(changed to dict, previously was an array)We can just delete this line, to fix the index error that would be thrown.
01a9813#diff-982c110867d9809c978a5597e4da66ce7538b327675a66b8f27214413b7c0ec1R643
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.
Another day, maybe I lost some context already, but clearing from
_on_query_completions_async
seems wrong because of this case:(isIncomplete == true)
LspResolveDocsCommand.completions
is cleared immediatelyLspResolveDocsCommand.completions
is empty.I think after the change to a dict it makes even more sense to clear it from the
_on_all_settled
because new completion request might not trigger for the same session names so at least from the memory/GC point of view it would make sense to clear that object at some point.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 moved it to
_on_all_settled
to avoid errors :)I guess the "ideal" place to clear the dict would be when the AC closes(either by the user selecting the completion item, or by pressing the esc key, or by pressing left right arrows). I know that there is a
is_auto_complete_visible()
api, but I don't yet have a simple solution for that approach(so I would avoid complicating the logic now).But I believe that moving it to
_on_all_settled
improves things a bit.