Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
@bobbrow Were all your concerns addressed? |
I haven't had the time to go through very many files yet. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 34 out of 37 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // clients, so the native process is already aware of all workspace | ||
| // folders before receiving new CppProperties for new folders. | ||
| this.defaultClient.sendDidChangeSettings(); | ||
| await this.defaultClient.sendDidChangeSettings(); |
There was a problem hiding this comment.
It might have been intentional that there was no await here. An await would allow other things to happen in parallel, potentially allowing other LSP messages through before the call to createClient a few lines down. I don't think awaiting sendDidChangeSettings is required. It's sending a notification as soon as ready. It might be safest to preserve the original behavior and use void instead of await.
There was a problem hiding this comment.
Sure, I can change it back to void, but comment makes it sound like it expected the settings to be send before hand though: send them up before creating new clients.
| const mappingString: string = baseFileName + "@" + document.fileName; | ||
| client.addFileAssociations(mappingString, "cpp"); | ||
| client.sendDidChangeSettings(); | ||
| await client.sendDidChangeSettings(); |
There was a problem hiding this comment.
This should not be awaited here. In the protocol handler, a call to await can results in other messages being processed, resulting in messages out of order (i.e. file-related requests before the didOpen is processed).
There was a problem hiding this comment.
I did that because otherwise it seemed like the setTextDocumentLanguage could happen after the sendDidChangeSettings, but I can change it back to void.
3 participants
This was mostly done by Copilot Agent mode...well, the initial edit...I had to fix a bunch of stuff it did.