code snippet provider #13018
code snippet provider #13018
Conversation
Extension/src/LanguageServer/copilotCompletionContextProvider.ts
Outdated
Show resolved
Hide resolved
Extension/src/LanguageServer/copilotCompletionContextProvider.ts
Outdated
Show resolved
Hide resolved
Extension/src/LanguageServer/copilotCompletionContextProvider.ts
Outdated
Show resolved
Hide resolved
| snippetsKind = kind; | ||
| // //?? TODO Add telemetry for Computed vs Cached. | ||
|
|
||
| return codeSnippets ?? []; |
There was a problem hiding this comment.
Won't matter if we're the only provider, but we may want to experiment with lowering the importance if the snippets were from the cache.
|
|
||
| export async function registerCopilotContextProvider(): Promise<void> { | ||
| try { | ||
| const isCustomSnippetProviderApiEnabled = await telemetry.isExperimentEnabled("CppToolsCustomSnippetsApi"); |
There was a problem hiding this comment.
We can't do audience filtering and set this flag at the same time. Instead, on the Copilot side you'll need to modify the construction of activeExperiments in CompletionContext to check for the experiment, then update the resolver here to respect activeExperiments.
Alternatively, if the intent of this check was to act as a kill-switch then you can leave it as-is (although we'll still need the activeExperiments plumbing). We'll just need to be sure that CppToolsCustomSnippetsApi is at 100% rollout for all VS Code traffic before starting the actual experiment.
Extension/src/LanguageServer/copilotCompletionContextProvider.ts
Outdated
Show resolved
Hide resolved
Extension/src/LanguageServer/copilotCompletionContextProvider.ts
Outdated
Show resolved
Hide resolved
|
@lukka Soon you should be able to take a dependency on https://www.npmjs.com/package/@github/copilot-language-server to get appropriate typings. The types aren't quite there yet, but https://github.com/github/copilot-client/pull/6250 is adding them. |
lukka
force-pushed
the
dev/lucappa/cop-ctx-api
branch
2 times, most recently
from
991df60 to
22235f3
Compare
Extension/src/LanguageServer/copilotCompletionContextProvider.ts
Outdated
Show resolved
Hide resolved
| import { randomUUID } from 'crypto'; | ||
| import * as telemetry from '../telemetry'; | ||
|
|
||
| export class CopilotContextTelemetry { |
There was a problem hiding this comment.
more appropriate name should be CopilotCompletionContextTelemetry
Extension/src/LanguageServer/copilotCompletionContextProvider.ts
Outdated
Show resolved
Hide resolved
| private readonly metrics: Record<string, number> = {}; | ||
| private readonly properties: Record<string, string> = {}; | ||
| private readonly id: string; | ||
| constructor(correlationId?: string) { |
There was a problem hiding this comment.
There should be a blank newline before constructor.
There was a problem hiding this comment.
I'll add it manually Should this be enforced by whatever the formatter is? I think I have enabled ESLint and formatting the document should be the "correct" one.
There was a problem hiding this comment.
I'm not sure which formatter we're using for TypeScript. We have "editor.defaultFormatter": "vscode.typescript-language-features", set in settings.json but we also have "eslint.format.enable": true, -- I've seen cases recently where the 2 formatters had different opinions and so I had to disable the vscode.typescript-language-features in order to fix the eslint issue.
I think eslint formatting is deprecated.
I'm not sure if there's a formatter rule for a newline before a member function, but feel free to add one if you know of a rule.
Extension/src/LanguageServer/copilotCompletionContextProvider.ts
Outdated
Show resolved
Hide resolved
Extension/src/LanguageServer/copilotCompletionContextProvider.ts
Outdated
Show resolved
Hide resolved
lukka
force-pushed
the
dev/lucappa/cop-ctx-api
branch
from
22235f3 to
0679bab
Compare
No description provided.