Skip to content

ContextProvider resolve handler improvements #13296

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.

Sign up for GitHub

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

Jump to bottom
Merged
lukka merged 1 commit into from
Mar 1, 2025
Merged

ContextProvider resolve handler improvements #13296

lukka merged 1 commit into from
Mar 1, 2025

Conversation

@lukka
Copy link
Member

@lukka lukka commented Feb 20, 2025

Changes:
-new feature: add traits for C++ lang version
-telemetry: fix cancellation events.
-telemetry: more diag event for registration failure.

@lukka lukka marked this pull request as ready for review February 20, 2025 18:16
@lukka lukka requested a review from a team as a code owner February 20, 2025 18:16
@lukka lukka changed the title ContextProvider resolved improvements ContextProvider resolve handler improvements Feb 20, 2025
benmcmorran
benmcmorran previously approved these changes Feb 20, 2025
View reviewed changes
sean-mcmanus
sean-mcmanus reviewed Feb 21, 2025
View reviewed changes
sean-mcmanus
sean-mcmanus requested changes Feb 21, 2025
View reviewed changes
Copy link
Contributor

@sean-mcmanus sean-mcmanus left a comment

Choose a reason for hiding this comment

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

Should SnippetEntry be removed? It doesn't appear to be referenced anymore?

@kuchungmsft
Copy link
Contributor

kuchungmsft commented Feb 24, 2025

FYI, I have a PR opened to clean up the compiler argument traits, which may impact this PR, see #13278.

@sean-mcmanus
Copy link
Contributor

sean-mcmanus commented Feb 24, 2025

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?

@lukka lukka force-pushed the dev/lukka/cs-improvements branch from 8c68910 to b0847de Compare February 26, 2025 01:06
kuchungmsft
kuchungmsft previously approved these changes Feb 26, 2025
View reviewed changes
sean-mcmanus
sean-mcmanus requested changes Feb 26, 2025
View reviewed changes
Copy link
Contributor

@sean-mcmanus sean-mcmanus left a comment •
edited
Loading

Choose a reason for hiding this comment

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

cppCodeSnippetsFeatureNames doesn't work if it's lowercased -- that seems a little error prone and unexpected, because the other C_Cpp settings values are case insensitive I believe.

sean-mcmanus
sean-mcmanus reviewed Feb 26, 2025
View reviewed changes
Copy link
Contributor

@sean-mcmanus sean-mcmanus left a comment

Choose a reason for hiding this comment

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

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.

sean-mcmanus
sean-mcmanus requested changes Feb 26, 2025
View reviewed changes
Copy link
Contributor

@sean-mcmanus sean-mcmanus left a comment

Choose a reason for hiding this comment

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

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?

@lukka lukka force-pushed the dev/lukka/cs-improvements branch from b0847de to 603f950 Compare February 26, 2025 08:06
kuchungmsft
kuchungmsft previously approved these changes Feb 26, 2025
View reviewed changes
sean-mcmanus

This comment was marked as resolved.

Sign in to view

sean-mcmanus
sean-mcmanus approved these changes Feb 26, 2025
View reviewed changes
Copy link
Contributor

@sean-mcmanus sean-mcmanus left a comment

Choose a reason for hiding this comment

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

@lukka Should the snippets be trimmed for whitespace?

image

sean-mcmanus
sean-mcmanus requested changes Feb 26, 2025
View reviewed changes
Copy link
Contributor

@sean-mcmanus sean-mcmanus left a comment

Choose a reason for hiding this comment

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

I still see unnecessary stuff sent to TypeScript like endLine. Can you remove that?

sean-mcmanus
sean-mcmanus approved these changes Feb 27, 2025
View reviewed changes
Copy link
Contributor

@sean-mcmanus sean-mcmanus left a comment

Choose a reason for hiding this comment

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

I'm approving but it seems like some of the data sent to TypeScript could be cleaned up on the cpptools side.

@lukka
Copy link
Member Author

lukka commented Feb 27, 2025

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.

sean-mcmanus
sean-mcmanus requested changes Feb 28, 2025
View reviewed changes
@lukka lukka force-pushed the dev/lukka/cs-improvements branch from 603f950 to f89aa92 Compare February 28, 2025 20:21
@lukka
-new feat: add traits for C++ lang version
0ea9532 
-telemetry: fix cancellation events.
-telemetry: more diag event for registration failure.
-add fallback to SimilarFiles providers such as openTabs and/or
related-files
@lukka lukka force-pushed the dev/lukka/cs-improvements branch from f89aa92 to 0ea9532 Compare February 28, 2025 20:53
@lukka lukka merged commit 1813c82 into main Mar 1, 2025
6 checks passed
@github-project-automation github-project-automation bot moved this from Pull Request to Done in cpptools Mar 1, 2025
@lukka lukka deleted the dev/lukka/cs-improvements branch March 1, 2025 02:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sean-mcmanus sean-mcmanus sean-mcmanus approved these changes

@benmcmorran benmcmorran benmcmorran left review comments

@kuchungmsft kuchungmsft kuchungmsft left review comments

Assignees

No one assigned

Labels

None yet

Projects

cpptools
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@lukka @kuchungmsft @sean-mcmanus @benmcmorran