-
Notifications
You must be signed in to change notification settings - Fork 226
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
Reproducer for S1192 FP: SQL Named Parameters #9593
Reproducer for S1192 FP: SQL Named Parameters #9593
Conversation
analyzers/tests/SonarAnalyzer.Test/TestCases/StringLiteralShouldNotBeDuplicated.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.
If this is relevant to the rule, I think you should put the dummy classes into the Dapper
namespace or the right namespace.
public class SqlCommand | ||
{ | ||
public string CommandText { get; } | ||
public SqlCommand(string commandText) => CommandText = commandText; | ||
public void AddParameter(SqlParameter parameter) { } | ||
public object ExecuteQuery() => null; | ||
} | ||
|
||
public class SqlParameter | ||
{ | ||
public string Name { get; } | ||
public string Value { get; } | ||
public SqlParameter(string name, string value) | ||
{ | ||
Name = name; | ||
Value = value; | ||
} | ||
} |
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.
To be sure, shouldn't those be in the Dapper
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 originally wanted to create some dummy classes, so that the UT doesn't need to load Dapper and its dependencies.
But a real-world example is probably better. I changed the test case and moved it to a dedicated test file.
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.
Optional: I think those test cases could be in a dedicated .Dapper.cs
file.
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 changed the test case and moved it to a dedicated test file.
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 VB dedicated file for Dapper is missing.
@@ -58,6 +58,16 @@ public class StringLiteralShouldNotBeDuplicatedTest | |||
.WithOptions(ParseOptionsHelper.FromCSharp11) | |||
.Verify(); | |||
|
|||
[TestMethod] | |||
public void StringLiteralShouldNotBeDuplicated_CSharp_Dapper() => |
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.
Nitpick
public void StringLiteralShouldNotBeDuplicated_CSharp_Dapper() => | |
public void StringLiteralShouldNotBeDuplicated_CS_Dapper() => |
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.
Is there any reason not to move those changes to a .Dapper.vb
file?
Quality Gate passed for 'Sonar .NET Java Plugin'Issues Measures |
Quality Gate passed for 'SonarAnalyzer for .NET'Issues Measures |
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!
Reproduces #9569