-
Notifications
You must be signed in to change notification settings - Fork 44
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
Fixed #89: allow user to customize rules xml #90
Conversation
- added option for the user to supply the rules.xml file to use, similar to the option for SQALE files.
<tag>t1</tag> | ||
<tag>t2</tag> | ||
<debtRemediationFunction>CONSTANT_ISSUE</debtRemediationFunction> | ||
<debtRemediationFunctionOffset>15min</debtRemediationFunctionOffset> |
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.
We don't currently generate template files files with tags or debt remediation data. However, this test checks that the serializer could correctly serialize that data. I'll add documentation to the readme explaining how to manually add these elements.
|
||
namespace SonarQube.Plugins.Roslyn | ||
{ | ||
public enum DebtRemediationFunctionType |
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.
SonarC# and SonarVB currently support only CONSTANT_ISSUE
so I am not sure if we will be able to provide the other values.
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'll add a comment saying they are unsupported by the C# plugin currently.
|
||
namespace SonarQube.Plugins.Roslyn | ||
{ | ||
public enum IssueType |
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.
Shall we allow external users to provide the new type (hotspot)?
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 plugin in LTS-compatible so I don't think hotspots are available.
} | ||
set | ||
{ | ||
this.DebtRemediationFunction = (DebtRemediationFunctionType)Enum.Parse(typeof(DebtRemediationFunctionType), 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.
Isn't it better to do a TryParse
and assign Unspecified
in case of wrong 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.
I think this is ok; we'll fail fast if the user supplies an invalid value.
LogMessageForGeneratedSqale(generatedSqaleFile); | ||
|
||
return createdJarFilePath; | ||
} | ||
|
||
private void LogMessageForGeneratedRules(string generatedFile) | ||
{ | ||
if (generatedFile != null) |
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.
Isn't it better to also log (another message) when the file is null?
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 that case the user will have supplied the file name as a parameter, and a message will have been logged in line 218 above.
/// </summary> | ||
private bool IsValidRulesFile(string filePath) | ||
{ | ||
Debug.Assert(!string.IsNullOrWhiteSpace(filePath)); |
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.
IMO those 2 assertions needs to be if/throw
.
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.
They're assertions because they are checking the validity of our code, not the user input. We don't expect our code to reach this point if the file is null or missing.
If the user has supplied a valid file then the code will already have thrown (see comment in line 419 below).
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.
Ok.
{ | ||
string expectedFilePath = GetExpectedRuleTemplateFilePath(outputDir, package); | ||
|
||
Assert.IsTrue(File.Exists(expectedFilePath), "Expecting a template rule file to have been created"); |
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.
Use fluent assertions instead.
private static void AssertRuleTemplateDoesNotExist(string outputDir) | ||
{ | ||
string[] matches = Directory.GetFiles(outputDir, "*rules*template*", SearchOption.AllDirectories); | ||
Assert.AreEqual(0, matches.Length, "Not expecting any rules template files to exist"); |
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.
Use fluent assertions instead.
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.
@duncanp-sonar You missed this one.
{ | ||
public class ProcessedArgsBuilder | ||
{ | ||
private string packageId; |
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.
Can be readonly
.
private string ruleFilePath; | ||
private bool acceptLicenses; | ||
private bool recurseDependencies; | ||
private string outputDirectory; |
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.
readonly
this.packageId = packageId; | ||
this.outputDirectory = outputDir; | ||
} | ||
public ProcessedArgsBuilder SetPackageVersion(string packageVersion) |
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.
Missing blank line before this method declaration.
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.
@@ -25,7 +25,8 @@ namespace SonarQube.Plugins.Roslyn.CommandLine | |||
{ | |||
public class ProcessedArgs | |||
{ | |||
public ProcessedArgs(string packageId, SemanticVersion packageVersion, string language, string sqaleFilePath, bool acceptLicenses, bool recurseDependencies, string outputDirectory) | |||
public ProcessedArgs(string packageId, SemanticVersion packageVersion, string language, string sqaleFilePath, string ruleFilePath, |
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.
Code Smell: Constructor has 8 parameters, which is greater than the 7 authorized. (csharpsquid:S107)
|
||
try | ||
{ | ||
// TODO: consider adding further checks |
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.
Code Smell: Complete the task associated to this 'TODO' comment. (csharpsquid:S1135)
The code follows the same pattern used for customizing SQALE i.e.