Refactor semantic snippets #74712
Refactor semantic snippets #74712
Conversation
ghost
added
Area-IDE
untriaged
dotnet-policy-service
bot
added
Community
| /// <summary> | ||
| /// The position that the cursor should end up on | ||
| /// </summary> | ||
| public readonly int FinalCaretPosition; |
There was a problem hiding this comment.
This is a rename CursorPosition -> FinalCaretPosition. First, I wanted to emphasize that this position is used after text changes and placeholders, so Final. And second, the standard term for roslyn is 'caret', not 'cursor', so made it more consistent with the rest of the codebase
| int cursorPosition, | ||
| ImmutableArray<SnippetPlaceholder> placeholders) | ||
| ImmutableArray<SnippetPlaceholder> placeholders, | ||
| int finalCaretPosition) |
There was a problem hiding this comment.
Reordered for clarity. We first insert text changes, then let user loop through placeholders and only then move user's caret to its final position
| /// Editable text in the snippet. | ||
| /// </summary> | ||
| public readonly string Identifier; | ||
| public readonly string Text; |
There was a problem hiding this comment.
This can be an actual identifier, but it also can be any other text, e.g. in if snippet the placeholder is a full condition expression. So Identifier is a confusing name for this thing, renamed it to more generic Text
| /// be converted into LSP formatted strings. | ||
| /// </summary> | ||
| public readonly ImmutableArray<int> PlaceHolderPositions; | ||
| public readonly ImmutableArray<int> StartingPositions; |
There was a problem hiding this comment.
Wanted to emphasize that these are starting positions for clarity. Also since they are already inside SnippetPlaceholder type, having PlaceHolder in the name is an overkill
|
@akhera99 PTAL |
I was working on another snippet-related PR and saw a few things that IMO can be improved. So I extracted all these refactorings into this PR. What have I done and why:
ISnippetProviderhadGetSnippetDataAsyncandGetSnippetAsyncmethods and for an external person it is somewhat confusing what is the difference between them. Moreover, from the nameGetSnippetDataAsyncI expect to get eigherTask<SnippetData>or aValueTask<SnippetData>, but notValueTask<SnippetData?>. By poolingAdditionalFilterTextsup to the interface I can replace confusingGetSnippetDataAsyncwithIsValidSnippetLocationAsyncand constructSnippetDatastruct when this method returnstruestaticto a few things