-
Notifications
You must be signed in to change notification settings - Fork 227
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
Rule S4041: Type names should not match namespaces #466
Conversation
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.
A couple of comments otherwise fine by me.
public sealed class TypeNamesShouldNotMatchNamespaces : SonarDiagnosticAnalyzer | ||
{ | ||
internal const string DiagnosticId = "S4041"; | ||
private const string MessageFormat = "Change the name of that type to be different from an existing namespace."; |
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 do understand now why they keep the list of namespaces and only split the name after. It is because it allows them to display which namespace conflicts with this name.
I would just add the name of the class/interface... in the message.
context.RegisterSyntaxNodeActionInNonGenerated(c => | ||
{ | ||
var declaration = (BaseTypeDeclarationSyntax)c.Node; | ||
var symbolDeclaredccess = c.SemanticModel.GetDeclaredSymbol(declaration)?.DeclaredAccessibility; |
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.
Typo
}, | ||
SyntaxKind.ClassDeclaration, | ||
SyntaxKind.InterfaceDeclaration, | ||
SyntaxKind.EnumDeclaration); |
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.
What about namespace
? What about struct
?
context.RegisterSyntaxNodeActionInNonGenerated(c => | ||
{ | ||
var declaration = (DelegateDeclarationSyntax)c.Node; | ||
var symbolDeclaredccess = c.SemanticModel.GetDeclaredSymbol(declaration)?.DeclaredAccessibility; |
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.
Typo
|
||
ReportIfNameClashesWithFrameworkNamespace(declaration?.Identifier, symbolDeclaredccess, c); | ||
}, | ||
SyntaxKind.DelegateDeclaration); |
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.
This block is exactly the same as the previous one except from the cast. I am pretty sure you could use c.SemanticModel.GetDeclaredSymbol(c.Node)
directly.
public interface Linq { } // Noncompliant | ||
// ^^^^ | ||
|
||
interface System { } // Compliant |
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 would add a comment about why this is compliant.
using System; | ||
|
||
namespace Tests.Diagnostics | ||
{ |
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.
This is going to be boring but I would add a test case for every name we handle (avoid regression).
Fix #436