Make middleware wait for connection #17879
Conversation
rchiodo
added
the
skip package*.json
| } | ||
| } | ||
|
|
||
| public resolveCompletionItem(): ProviderResult<CompletionItem> { |
There was a problem hiding this comment.
Was there any reason to remove the return type?
There was a problem hiding this comment.
None of the others do, it seems.
There was a problem hiding this comment.
So that I didn't have to rewrite it as Promise. It's inferred from the other call so we don't really need it.
| @@ -0,0 +1 @@ | |||
| Semantic colorization can sometimes require reopening or scrolling of a file. No newline at end of file | |||
There was a problem hiding this comment.
This seems like an odd changelog item; does this fix that?
There was a problem hiding this comment.
Yes. The root cause was the provideDocumentSemanticTokens override I added for notebooks. If connected was false, the token request would return null
There was a problem hiding this comment.
I only ask because I think most changelog items say something like "Fixed bug ..." or something.
| public provideHover() { | ||
| if (this.connected) { | ||
| public async provideHover() { | ||
| if (await this.connected) { |
There was a problem hiding this comment.
Why do some methods await connected, while some just call callNext?
There was a problem hiding this comment.
The ones that don't await connected don't return promises. They can't await. And they should honestly go through before connection anyway.
|
@jakebailey did you have any more comments? |
|
I still don't understand why you need to do all of this in the middleware. Shouldn't this be the responsibility of the server since the server knows which state it has to reach to correctly answer a request. |
The server doesn't know what the interpreter selected in VS code is. It's waiting for that to be computed. |
|
Further, no Python LS has knowledge of notebooks (interpreter is one aspect, but on the whole, we don't know how to piece together cells or anything), so this is all required to let the Jupyter extension and such deal with triaging requests to handle them correctly. Our intent is that all of this code gets dropped as we work to shift notebook support down the stack into Pylance. |
I like this since it is the right approach. Would be cool if the Python extension talks to the Pylance extension using our command based language API (see https://code.visualstudio.com/api/references/commands) |
This is actually where I was starting (using commands) (example: there's no LSP message for notebook cells moving around). Good to know that you agree :) |
5 participants
Fixes #17878
We can't skip token requests just because the python interpreter isn't set yet in pylance. Instead we should wait for the interpreter to be set.