SA1413 now also checks enum members. #2245
Conversation
Current coverage is 96.84% (diff: 100%)@@ master #2245 diff @@
==========================================
Files 598 598
Lines 84026 84059 +33
Methods 3581 3580 -1
Messages 0 0
Branches 3196 3197 +1
==========================================
+ Hits 81369 81404 +35
+ Misses 1790 1789 -1
+ Partials 867 866 -1
|
| if (initializer.Members.Count() != commas.Count()) | ||
| { | ||
| context.ReportDiagnostic(Diagnostic.Create(Descriptor, lastMember.GetLocation())); | ||
| } |
There was a problem hiding this comment.
The code above should follow the same pattern as the methods below. Specifically you can check EnumDeclarationSyntax.Members.Count against EnumDeclarationSyntax.Members.SeparatorCount. That will work just as well and it will perform quite a bit better.
There was a problem hiding this comment.
Thanks for pointing that out. I was looking for something like this 👍
| await this.VerifyCSharpDiagnosticAsync(fixedTestCode, EmptyDiagnosticResults, CancellationToken.None).ConfigureAwait(false); | ||
| await this.VerifyCSharpFixAsync(testCode, fixedTestCode).ConfigureAwait(false); | ||
| } | ||
|
|
There was a problem hiding this comment.
Please add test cases where the element without comma is directly followed by an end of line comment. Like:
var testCode = @"enum TestEnum
{
One = 2 /* test comment */
}
";and verify that it is properly changed to
var testCode = @"enum TestEnum
{
One = 2, /* test comment */
}
";There was a problem hiding this comment.
I changed my unit test and it still passes
| { | ||
| var member = (EnumMemberDeclarationSyntax)syntaxRoot.FindNode(diagnostic.Location.SourceSpan); | ||
| return new TextChange(diagnostic.Location.SourceSpan, member.ToString() + ","); | ||
| } |
There was a problem hiding this comment.
💭 The code above should be the same pattern as the code for RewriteInitializer and RewriteAnonymousObjectInitializer.
If this code performs better than the existing code, then please also change the other methods to use the same pattern, for consistency.
There was a problem hiding this comment.
I changed the code to only use my method and it still passes all the unit tests. Not sure how to test if it performs better but it does a lot less work: f983b5f#diff-9ac99be9ffc83f0bfb6aa1415e965bccR49.
|
|
||
| private static TextChange GetEnumMemberTextChange(Diagnostic diagnostic, SyntaxNode syntaxRoot) | ||
| { | ||
| var member = (EnumMemberDeclarationSyntax)syntaxRoot.FindNode(diagnostic.Location.SourceSpan); |
There was a problem hiding this comment.
The syntaxRoot.FindNode call has already been done in the calling method, please reuse the result of that, as this is usually a quite expensive call.
|
Thanks for the quick review. Will apply the changes this weekend. (or earlier 😛 ) |
SA1413 now also checks enum members.
This patch implements the feature that was requests in #2239.
Fixes #2239