Skip to content
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

Implement context provider #7921

Closed
wants to merge 2 commits into from
Closed

Implement context provider #7921

wants to merge 2 commits into from

Conversation

genlu
Copy link
Member

@genlu genlu commented Jan 11, 2025

No description provided.

const ext = vscode.extensions.getExtension('github.copilot');
if (!ext) {
channel.debug('GitHub Copilot extension not installed. Skip registeration of C# related files provider.');
return;
return undefined;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would keep the log messages if possible (trace/debug only though)

try {
return await ext.activate();
} catch {
return undefined;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the catch necessary here? There is already a .catch in the caller which also logs the error.
If you're keeping this catch here, it definitely needs to log the error.

} catch {
return undefined;
}
} else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need for the if else here. activate should no-op if already active

selector: [{ language: 'csharp' }],
resolver: {
resolve: async (request, token) => {
return [{ uri: 'testUri', value: 'testValue', additionalUris: [] }];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assuming this is the part that will eventually call out to the server

return undefined;
}
} else {
return ext.exports as CopilotAPIs | undefined;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

guessing this fails if the interface definition doesn't match? e.g. the version of the extension doesn't have a getContextProviderAPI export?

@genlu genlu closed this Feb 6, 2025
@genlu genlu deleted the ContextProvider branch February 6, 2025 01:28
@genlu
Copy link
Member Author

genlu commented Feb 6, 2025

Closed in favor of an alternative approach

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants