-
Notifications
You must be signed in to change notification settings - Fork 30
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
Create rule S1312: Add C# #2488
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.
The rule, as defined, seems to make sense to me.
There are just some improvements to be done to wording and explanations.
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.
LGTM! Good use of variables, to support the variance of keywords.
I left a couple of discussions open, to fix some minor issues.
rules/S1312/rule-dotnet.adoc
Outdated
|
||
This rule should be activated when https://en.wikipedia.org/wiki/Service_locator_pattern[Service Locator Design pattern] is followed in place of https://en.wikipedia.org/wiki/Dependency_injection[Dependency Injection] for logging. | ||
|
||
The rule will support the most popular logging frameworks: |
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.
will support
-> supports
rules/S1312/csharp/resources.adoc
Outdated
@@ -0,0 +1,3 @@ | |||
* Microsoft Learn - https://learn.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/access-modifiers[Access modifiers] | |||
* Microsoft Learn - https://learn.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/static-classes-and-static-class-members[Static class members] |
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.
In the VB.NET version of resources Shared
is considered a keyword (i.e. it's backticked), but here static
it is not (second link).
I would change here, to use backtick as for VB.NET.
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.
In addition: should/can we use the adoc variables, defined at /rule.adoc level?
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 don't think we should considering that those resources are already language specific, but this is not a strong opinion
SonarQube Quality Gate for 'rspec-frontend' |
SonarQube Quality Gate for 'rspec-tools' |
|
|
You can preview this rule here (updated a few minutes after each push).
Review
A dedicated reviewer checked the rule description successfully for: