-
Notifications
You must be signed in to change notification settings - Fork 231
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
Fix S2094 FP: Allow to have empty class with non-empty ctor for base class #6960
Fix S2094 FP: Allow to have empty class with non-empty ctor for base class #6960
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.
Nicely done. Only some minor comments.
analyzers/tests/SonarAnalyzer.UnitTest/TestCases/ClassShouldNotBeEmpty.CSharp9.cs
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
Kudos for reorganizing the test cases!
There are some merge conflicts that need to be resolved.
970b2cf
to
93cc46d
Compare
@zsolt-kolbay-sonarsource Rebase done. |
/azp run Sonar.Net |
Azure Pipelines successfully started running 1 pipeline(s). |
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.
thanks @Corniel - I've left some last comments
analyzers/src/SonarAnalyzer.CSharp/Rules/ClassShouldNotBeEmpty.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.CSharp/Rules/ClassShouldNotBeEmpty.cs
Outdated
Show resolved
Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/TestCases/ClassShouldNotBeEmpty.CSharp10.cs
Outdated
Show resolved
Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/TestCases/ClassShouldNotBeEmpty.CSharp10.cs
Outdated
Show resolved
Hide resolved
@andrei-epure-sonarsource Did all of them. |
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!
(note: I didn't check in detail each of the test cases that they got moved, I just sampled some)
/azp run Sonar.Net |
Azure Pipelines successfully started running 1 pipeline(s). |
91cf77e
to
1edd576
Compare
@zsolt-kolbay-sonarsource while reviewing some code with a collegae, I spotted a false negative for this rule: record EmptyChildWithoutBrackets : EmptyRecord; // Noncompliant While at it, I extended the rule to also cover this. |
/azp run Sonar.Net |
Azure Pipelines successfully started running 1 pipeline(s). |
The fix for the FN requires an update of the ITs. Once you've done that I'll merge the PR. |
@zsolt-kolbay-sonarsource Can you do that for me please? |
/azp run Sonar.Net |
Azure Pipelines successfully started running 1 pipeline(s). |
Kudos, SonarCloud Quality Gate passed! |
Kudos, SonarCloud Quality Gate passed! |
Fixes #6953
Also fixes FN for empty record w/o brackets.