-
Notifications
You must be signed in to change notification settings - Fork 232
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
Add C#9 tests for S1186, S1116, S2291 , S2197, S4035, S4220 #3749
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.
LGTM! Just a few minor comments.
[TestCategory("Rule")] | ||
public void EmptyMethod_CSharp9() | ||
{ | ||
Verifier.VerifyAnalyzer(@"TestCases\EmptyMethod.CSharp9.cs", |
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.
It was intended to use this instead of VerifyAnalyzerFromCSharp9Console
?
{ | ||
public string Prop | ||
{ | ||
init { } |
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.
Shouldn't this be a FN since it's not an auto-implemented property?
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 current rule only looks at methods, so it's the same behavior as for set
.
I've opened #3753 to do it
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.
Shouldn't we mark both as FNs then?
SomeStaticEvent?.Invoke(this, e); // Noncompliant {{Make the sender on this static event invocation null.}} | ||
SomeStaticEvent?.Invoke(null, null); // Noncompliant {{Use 'EventArgs.Empty' instead of null as the event args of this event invocation.}} | ||
SomeStaticEvent?.Invoke(this, null); // Noncompliant | ||
// Noncompliant@-1 |
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.
Should we add some cases with direct invocation like below?
SomeEvent(null, e);
SomeStaticEvent(null, null);
4290110
to
a96dee2
Compare
Kudos, SonarCloud Quality Gate passed!
|
Kudos, SonarCloud Quality Gate passed!
|
Related to #3668