-
Notifications
You must be signed in to change notification settings - Fork 239
New rule S6960 for C#: Controllers should not have too many responsibilities #9111
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
New rule S6960 for C#: Controllers should not have too many responsibilities #9111
Conversation
c109ac7 to
eeb986b
Compare
gregory-paidis-sonarsource
left a comment
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.
Amazing work, very clever and clean implementation 💘
Left some questions and some cleaning-up suggestions.
I did not go over the test cases again, I trust that you copied what was already reviewed on the RSPEC side.
If there are changes, please let me know with a comment where I should look.
analyzers/tests/SonarAnalyzer.Test/Rules/AspNet/ControllersHaveTooManyResponsibilitiesTest.cs
Outdated
Show resolved
Hide resolved
analyzers/tests/SonarAnalyzer.Test/Rules/AspNet/ControllersHaveTooManyResponsibilitiesTest.cs
Outdated
Show resolved
Hide resolved
analyzers/tests/SonarAnalyzer.Test/Rules/AspNet/ControllersHaveTooManyResponsibilitiesTest.cs
Outdated
Show resolved
Hide resolved
analyzers/tests/SonarAnalyzer.Test/Rules/AspNet/ControllersHaveTooManyResponsibilitiesTest.cs
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.CSharp/Rules/AspNet/ControllersHaveTooManyResponsibilities.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.CSharp/Rules/AspNet/ControllersHaveTooManyResponsibilities.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.CSharp/Rules/AspNet/ControllersHaveTooManyResponsibilities.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.CSharp/Rules/AspNet/ControllersHaveTooManyResponsibilities.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.CSharp/Rules/AspNet/ControllersHaveTooManyResponsibilities.cs
Outdated
Show resolved
Hide resolved
15ce7c2 to
c4ac7ce
Compare
|
@gregory-paidis-sonarsource I have undrafted the PR, after changing the base to the PR with the Disjoint Sets data structure. |
analyzers/src/SonarAnalyzer.CSharp/Rules/AspNet/ControllersHaveTooManyResponsibilities.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.CSharp/Rules/AspNet/ControllersHaveTooManyResponsibilities.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.CSharp/Rules/AspNet/ControllersHaveTooManyResponsibilities.cs
Outdated
Show resolved
Hide resolved
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, only one open comment remaining from me which is optional
…and ignore static fields and methods
878c0c0 to
c338f64
Compare
|
|
@gregory-paidis-sonarsource BTW, I'd ignore the code smell as it won't fix for us. |
…y to increase coverage
d7fe7cf to
738eb0b
Compare
|
|
gregory-paidis-sonarsource
left a comment
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




Fixes #9089