Update S101: A special case should be made for two-letter acronyms in… #577
Conversation
|
@michalb-sonar let's put link to ticket using issue reference (#). |
| { | ||
| "id": "S101", | ||
| "message": "Rename interface 'ITVImageScraper' to match camel case naming rules, consider using 'ITvImageScraper'.", | ||
| "message": "Rename interface 'ITVImageScraper' to match camel case naming rules, consider using 'ItvImageScraper'.", |
There was a problem hiding this comment.
For me we should not raise at all here:
I for interface, TV, Image, Scraper
We might want to detect whether this is an interface to allow/prevent this. What I mean is, in this case, it's ok if this is an interface, it's not otherwise.
| @@ -28,7 +15,7 @@ | |||
| }, | |||
| { | |||
| "id": "S101", | |||
| "message": "Rename interface 'ITVInfoScraper' to match camel case naming rules, consider using 'ITvInfoScraper'.", | |||
| "message": "Rename interface 'ITVInfoScraper' to match camel case naming rules, consider using 'ItvInfoScraper'.", | |||
| @@ -15,7 +15,7 @@ | |||
| }, | |||
| { | |||
| "id": "S100", | |||
| "message": "Rename method 'expectMsgPF' to match camel case naming rules, consider using 'ExpectMsgPF'.", | |||
| "message": "Rename method 'expectMsgPF' to match camel case naming rules, consider using 'ExpectMsgPf'.", | |||
There was a problem hiding this comment.
Haven't read the full description of the rule but is it allowed to have 2 letters acronym only if not at the end? If yes then fine, otherwise we have a FP here.
There was a problem hiding this comment.
I thought exactly the same. However, this doesn't raise because of two letter acronym, but because it doesn't start with uppercase letter.
There was a problem hiding this comment.
Ah! Shall we not have a suggestion with the least possible change? (i.e. ExpectMsgPF)
There was a problem hiding this comment.
I've tried that, but then there are suboptimal cases - like we would suggest "FSm" instead of "FSM". I'll leave it for now, we can always revise it.
|
|
||
| private static readonly Dictionary<SyntaxKind, string> TypeKindNameMapping = new Dictionary<SyntaxKind, string> | ||
| { | ||
| {SyntaxKind.StructDeclaration, "struct" }, |
There was a problem hiding this comment.
That's minor but I would have added a space after the opening curly braces.
| : input; | ||
| } | ||
|
|
||
| private static bool IsCharUpper(string input, int idx) |
| bool isNameValid = IsTypeNameValid(identifier.ValueText, | ||
| typeDeclaration is InterfaceDeclarationSyntax, | ||
| typeDeclaration.Modifiers.Any(m => m.IsKind(SyntaxKind.StaticKeyword)), | ||
| context.SemanticModel.Compilation.IsTest(), |
There was a problem hiding this comment.
Try to use the short form here: context.IsTest().
| continue; | ||
| } | ||
|
|
||
| var ideal = i == 0 ? HandleFirstTypePart(part, requireInitialI, allowInitialI, 1) |
There was a problem hiding this comment.
[minor] I would put ? on new line.
| var ideal = i == 0 ? HandleFirstTypePart(part, requireInitialI, allowInitialI, 1) | ||
| : SuggestLessUppercase(part, 1); | ||
|
|
||
| var acceptable = i == 0 ? HandleFirstTypePart(part, requireInitialI, allowInitialI, 2) |
There was a problem hiding this comment.
[minor] I would put ? on new line.
| var idealNameVariant = new StringBuilder(identifierName.Length); | ||
| var acceptableNameVariant = new StringBuilder(identifierName.Length); | ||
|
|
||
| var parts = SplitToCamelCase(identifierName).ToList(); |
There was a problem hiding this comment.
These are not words. It could also be a symbol, or a two-letter acronym, which makes it a bit confusing.
| var parts = SplitToCamelCase(identifierName).ToList(); | ||
| for (int i = 0; i < parts.Count; i++) | ||
| { | ||
| string part = parts[i]; |
There was a problem hiding this comment.
What about currentWord?
| typeDeclaration.Modifiers.Any(m => m.IsKind(SyntaxKind.StaticKeyword)), | ||
| context.SemanticModel.Compilation.IsTest(), | ||
| out suggestion); | ||
| requireInitialI: typeDeclaration is InterfaceDeclarationSyntax, |
There was a problem hiding this comment.
So much easier to read :)
… which both letters are capitalized
Fix S101: "Class Name" infinite loop when class name contains non-Latin Characters
Fix S100: infinite loop when class name contains non-Latin Characters
Fix #174
Fix #576
Fix #526