Properly preserve doc comments when converting a property into a method. #18235
Conversation
| /// Gets the active workspace project context that provides access to the language service for the active configured project. | ||
| /// </summary> | ||
| /// <value> | ||
| /// An value that provides access to the language service for the active configured project. |
There was a problem hiding this comment.
"An value" -> "A value", just because it's driving me nuts.
There was a problem hiding this comment.
TestDocumentationCommentFromGetterOnlyAutoProperty
There was a problem hiding this comment.
I often find test method names pointless. I want the test content to simply be self descriptive. If i could make most tests anonymous, i would. :)
There was a problem hiding this comment.
As a frequent reviewer of your tests, I don't find them self-descriptive. Having the name call out the interesting differentiating thing that is the reason this particular test exists is valuable. Otherwise, reviewers are left mentally diffing the test to all of the other ones to try to understand it's purpose.
|
|
||
| [WorkItem(18234, "https://github.com/dotnet/roslyn/issues/18234")] | ||
| [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsReplacePropertyWithMethods)] | ||
| public async Task TestDocumentationComment2() |
There was a problem hiding this comment.
TestDocumentationCommentFromSetterOnlyAutoProperty
|
|
||
| [WorkItem(18234, "https://github.com/dotnet/roslyn/issues/18234")] | ||
| [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsReplacePropertyWithMethods)] | ||
| public async Task TestDocumentationComment3() |
There was a problem hiding this comment.
TestDocumentationCommentFromAutoProperty
|
|
||
| <WorkItem(18235, "https://github.com/dotnet/roslyn/pull/18235")> | ||
| <Fact, Trait(Traits.Feature, Traits.Features.CodeActionsReplacePropertyWithMethods)> | ||
| Public Async Function TestDocumentationComment1() As Task |
There was a problem hiding this comment.
Same comments about test names as on the C# side.
| CancellationToken cancellationToken) | ||
| { | ||
| var methodDeclaration = GetSetMethodWorker( | ||
| generator, propertyDeclaration, propertyBackingField, | ||
| setMethod, desiredSetMethodName, cancellationToken); | ||
|
|
||
| methodDeclaration = CopyLeadingTrivia(propertyDeclaration, methodDeclaration, copyLeadingTrivia); |
There was a problem hiding this comment.
Seems weird to pass a boolean into a method telling the method whether it should do anything. Why not check the boolean here? I had to go looking to see if the call had side effects when copyLeadingTrivia is false, and it would have been easier if it just wasn't called in that case. methodDeclaration = copyLeadingTrivia ? CopyLeadingTrivia(propertyDeclaration, methodDeclaration) : methodDeclaration?
| { | ||
| public static readonly CSharpSyntaxRewriter Instance = new ConvertValueToReturnsRewriter(); | ||
|
|
||
| private ConvertValueToReturnsRewriter() |
| Return methodDeclaration | ||
| End Function | ||
|
|
||
| Private Function CopyLeadingTriviaOver(propertyStatement As PropertyStatementSyntax, |
There was a problem hiding this comment.
You just called this CopyLeadingTrivia on the C# side (sans the "Over"). I think I prefer the non-"Over" name.
There was a problem hiding this comment.
I can't call it CopyLeadingTrivia in VB because then it would conflict with the local of that name :-/
| { | ||
| private class ConvertValueToReturnsRewriter : CSharpSyntaxRewriter | ||
| { | ||
| public static readonly CSharpSyntaxRewriter Instance = new ConvertValueToReturnsRewriter(); |
There was a problem hiding this comment.
Fields should start with a lowercase letter, right? At a minimum, the capitalization of this is inconsistent between C# and VB.
| node, | ||
| Function(d) | ||
| Dim propertyStatement = DirectCast(d, PropertyStatementSyntax) | ||
| Dim mods = SyntaxFactory.TokenList(propertyStatement.Modifiers.Where(Function(tk) tk.IsKind(SyntaxKind.ReadOnlyKeyword) Or tk.IsKind(SyntaxKind.DefaultKeyword))) |
There was a problem hiding this comment.
mods -> modifiers; tk -> token
Fixes #18234