Register Copilot relatedFilesProvider for C#
#7578
Conversation
src/lsptoolshost/copilot.ts
Outdated
| } | ||
| await ext.activate(); | ||
| const relatedAPI = ext.exports as | ||
| | { |
There was a problem hiding this comment.
Can you create a separate type definition for this? It's a little hard to read inline
src/lsptoolshost/copilot.ts
Outdated
| | undefined; | ||
| if (!relatedAPI) { | ||
| channel.appendLine( | ||
| 'Incompatible GitHub Copilot extension installed. Skipping call to `registerRelatedFilesProvider`' |
There was a problem hiding this comment.
It is possible to show a notification asking the user to upgrade to a new version if you think it would be valuable. But I'd only do that if the extension is already installed
src/lsptoolshost/copilot.ts
Outdated
| export async function registerCopilotExtension(languageServer: RoslynLanguageServer, channel: vscode.OutputChannel) { | ||
| const ext = vscode.extensions.getExtension('github.copilot'); | ||
| if (!ext) { | ||
| channel.appendLine('GitHub Copilot extension not installed. Skipping call to `registerRelatedFilesProvider`'); |
There was a problem hiding this comment.
These are technically user visible strings. Consider making it more evident want this actually means in terms of feature set
There was a problem hiding this comment.
hmm, maybe I should move this to the trace log. This is intended to provide additional context implicitly (i.e. no user facing option to turn it on and off)
There was a problem hiding this comment.
Also fine with this being written when trace logging is enabled. I would still write to the C# output window though
src/lsptoolshost/copilot.ts
Outdated
|
|
||
| relatedAPI.registerRelatedFilesProvider(id, async (uri) => { | ||
| const writeOutput = (output: CopilotRelatedDocumentsReport[], builder: vscode.Uri[] | null) => { | ||
| if (output) { |
There was a problem hiding this comment.
If the output is actually nullable, it probably should be output?: CopilotRelatedDocumentsReport[]. Otherwise this check should be removed.
The client response type is defined as non-null in roslynProtocol, but the server defines the response as nullable -https://github.com/dotnet/roslyn/pull/74918/files#diff-0b28133c769dc0e92c52213acbee40648526328ab9cec6c7c0e2a9b5a43fa76bR70
It doesn't look like the server ever returns null though - so potentially the server annotation needs to be updated?
src/lsptoolshost/copilot.ts
Outdated
| } | ||
| ); | ||
|
|
||
| await responsePromise.then( |
There was a problem hiding this comment.
I suspect the other places using languageServer.sendRequestWithProgress need to be updated - but I think it should be fine to just await it directly (in a try catch if you want to handle errors).
|
@dibarbet Could you please take another look? (also I think this doesn't require dotnet/roslyn#75176 to work, since the protocol is still compatible with change in this PR, pls correct me if I was wrong) |
There was a problem hiding this comment.
@dibarbet Could you please take another look? (also I think this doesn't require dotnet/roslyn#75176 to work, since the protocol is still compatible with change in this PR, pls correct me if I was wrong)
I believe so, since it was optional.
| @@ -1036,6 +1037,7 @@ export async function activateRoslynLanguageServer( | |||
| ); | |||
|
|
|||
| registerLanguageStatusItems(context, languageServer, languageServerEvents); | |||
| await registerCopilotExtensionAsync(languageServer, _traceChannel); | |||
There was a problem hiding this comment.
I'd recommend not using the trace channel - that is generally just for LSP trace logs. What it should do is write to the normal output channel only when trace logging is enabled
registerRelatedFilesProvideris a new method on the copilot extension.RelatedDocumentsHandleris implemented here in Roslyn language server.