ContextProvider resolve handler improvements#13296
Conversation
lukka
changed the title
Extension/src/LanguageServer/copilotCompletionContextProvider.ts
Outdated
Show resolved
Hide resolved
Extension/src/LanguageServer/copilotCompletionContextProvider.ts
Outdated
Show resolved
Hide resolved
sean-mcmanus
left a comment
sean-mcmanus
left a comment
There was a problem hiding this comment.
Should SnippetEntry be removed? It doesn't appear to be referenced anymore?
Extension/src/LanguageServer/copilotCompletionContextProvider.ts
Outdated
Show resolved
Hide resolved
|
FYI, I have a PR opened to clean up the compiler argument traits, which may impact this PR, see #13278. |
Should your PR get checked in first, right? |
Extension/src/LanguageServer/copilotCompletionContextProvider.ts
Outdated
Show resolved
Hide resolved
lukka
force-pushed
the
dev/lukka/cs-improvements
branch
from
8c68910 to
b0847de
Compare
sean-mcmanus
left a comment
sean-mcmanus
left a comment
There was a problem hiding this comment.
Oh, but that doesn't appear to be a regression -- it looks like I modified the setting value to be lowercase and expected it to work.
Extension/src/LanguageServer/copilotCompletionContextProvider.ts
Outdated
Show resolved
Hide resolved
sean-mcmanus
left a comment
sean-mcmanus
left a comment
There was a problem hiding this comment.
getEnabledFeatureNames splits by "," and breaks if ", " with a space after the comma is used -- that seems error prone. Should it be changed to handle space after a comma?
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
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/lukka/cs-improvements
branch
from
b0847de to
603f950
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Extension/src/LanguageServer/copilotCompletionContextProvider.ts
Outdated
Show resolved
Hide resolved
Extension/src/LanguageServer/copilotCompletionContextProvider.ts
Outdated
Show resolved
Hide resolved
sean-mcmanus
left a comment
sean-mcmanus
left a comment
There was a problem hiding this comment.
@lukka Should the snippets be trimmed for whitespace?
sean-mcmanus
left a comment
sean-mcmanus
left a comment
There was a problem hiding this comment.
I still see unnecessary stuff sent to TypeScript like endLine. Can you remove that?
sean-mcmanus
left a comment
sean-mcmanus
left a comment
There was a problem hiding this comment.
I'm approving but it seems like some of the data sent to TypeScript could be cleaned up on the cpptools side.
Thanks! I have fixed and cleanup most (if not all) the things you suggested. The only one I am not going to do is the trimming of the whitespace. That should be unnecessary, and I am going to ask to the Copilot experts before doing it. In any case, not in this PR. |
Extension/src/LanguageServer/copilotCompletionContextTelemetry.ts
Outdated
Show resolved
Hide resolved
lukka
force-pushed
the
dev/lukka/cs-improvements
branch
from
603f950 to
f89aa92
Compare
-telemetry: fix cancellation events. -telemetry: more diag event for registration failure. -add fallback to SimilarFiles providers such as openTabs and/or related-files
lukka
force-pushed
the
dev/lukka/cs-improvements
branch
from
f89aa92 to
0ea9532
Compare
4 participants
Changes:
-new feature: add traits for C++ lang version
-telemetry: fix cancellation events.
-telemetry: more diag event for registration failure.