-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Semantic snippets - svm and sim snippets
#68854
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
Conversation
|
@akhera99 PTAL |
akhera99
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks for the contribution!!!
|
|
||
| namespace Microsoft.CodeAnalysis.CSharp.Snippets | ||
| { | ||
| [ExportSnippetProvider(nameof(ISnippetProvider), LanguageNames.CSharp), Shared] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ Is this name correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure what exactly name you mean here (ExportSnippetProvider or ISnippetProvider or something else), but everything should be right. You can verify it by pulling the latest main and verifying it on your own in VS hive. I always do that before submitting PRs, but I am not currently at my main main PC, so I cannot immediately provide the gif
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
➡️ It looks like nameof(ISnippetProvider) goes to a location which is ignored, and it just happens that all of the values are wrong in the code base. The intended use of a name in exporting would be to use nameof(CSharpIntMainSnippetProvider) or something similar. However, since the value is ignored and consistent with all the other cases, we can keep it like this for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 It would be acceptable to send a PR to remove the Name property from ExportSnippetProviderAttribute.
|
|
||
| namespace Microsoft.CodeAnalysis.CSharp.Snippets | ||
| { | ||
| [ExportSnippetProvider(nameof(ISnippetProvider), LanguageNames.CSharp), Shared] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ Is this name correct?
Commit 1: Added
svmandsimsnippetsCommits 2 and 3: I realized that there are many snippets that place caret target position inside a block. So I reused helper class (first renamed it, since it now deals not only with indentation) for common helper method for that
@akhera99 for review