-
Notifications
You must be signed in to change notification settings - Fork 148
Conversation
Thanks! Since I already refactored all parsers to use the new API of analysis-model it is important, that no Jenkins libraries are used by a parser. Can you please refactor the code so that the following library is not used?
You can add new dependencies like
|
String modulePath = ""; | ||
String moduleKey = component.optString(COMPONENT_MODULE_KEY, ""); | ||
JSONObject moduleComponent = findComponentByKey(components, moduleKey); | ||
if (moduleComponent.containsKey(COMPONENT_PATH)) { |
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.
Will throw NPE if findComponentByKey returns NULL
if (content instanceof JSONObject) { | ||
JSONObject jsonReport = (JSONObject) content; | ||
//Get the components part to get the file paths on each issue (the component objects contain the most concise path) | ||
JSONArray components = 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.
Don't use null, Optional can handle this much better (extract into new method)
if (moduleComponent.containsKey(COMPONENT_PATH)) { | ||
modulePath = moduleComponent.getString(COMPONENT_PATH) + "/"; | ||
} | ||
|
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.
extract code to get modulePath into method
} else if (severity.equals(SEVERITY_MINOR) || severity.equals(SEVERITY_INFO)) { | ||
priority = Priority.LOW; | ||
} | ||
} |
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.
extract code to get priority into method
for (Object component : components) { | ||
if (component instanceof JSONObject) { | ||
JSONObject jsonComponent = (JSONObject)component; | ||
if (jsonComponent.getString(COMPONENT_KEY).equals(key)) { |
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.
flip equals to avoid NPE
modulePath = moduleComponent.getString(COMPONENT_PATH) + "/"; | ||
} | ||
} | ||
|
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.
Similar to other class, does it make sense to use a common base?
|
||
return 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.
Extract common features into base class
|
||
Warnings.SonarQubeDiff.ParserName=SonarQube Diff | ||
Warnings.SonarQubeDiff.LinkName=SonarQube Diff | ||
Warnings.SonarQubeDiff.Trend=SonarQube Diff Warnings Trend |
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.
You can have icons and a default file name pattern in the next major release, does it make sense to define one for SonarQube?
See example.
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.
That'd be nice, maybe put these icons from the SQ scanner plugin?
And a default pattern of "**/target/sonar/sonar-report.json".
Should I change branch and extend StaticAnalysisTool for that though? :'D
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.
That depends on how much spare time you have ;-) I need to convert the parser to the new API anyway (see https://github.com/jenkinsci/analysis-model). Adding new parsers is now a little bit more complex since the source code is split into two different projects, one for the parsers (no Jenkins dependency) and one for the UI (Jenkins only).
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.
Frankly, I don't have much time on my hands, but maybe I can do it. What's there to be done? :0
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.
Basically, the same PR for the https://github.com/jenkinsci/analysis-model project. You need to extend from AbstractParser and need to create a Report. The test needs to extend from AbstractParserTest. The rest should be the same...
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, I'll make the changes to the analysis-model project and PR. Is there anything else to do in this PR? :0 Thank you for your attention!
* @author Carles Capdevila <Carles.Capdevila@t-systems.com> | ||
*/ | ||
@Extension | ||
public class SonarqubeDiffParser extends AbstractWarningsParser { |
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.
CamelCase SonarQube
* @author Carles Capdevila <Carles.Capdevila@t-systems.com> | ||
*/ | ||
@Extension | ||
public class SonarqubeIssuesParser extends AbstractWarningsParser { |
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.
CamelCase SonarQube
Thank you! I'll get to work on the requested changes and get back to you. |
A first beta release of the warnings plug-in is now available. Currently I only integrated the SonarQubeIssuesParser. It would be nice if we can fix that before the 5.0 release. |
I'm on vacation for a couple weeks, so I don't think I can work on this until at least next month. Is there any estimation for releasing 5.0 yet? Sorry about that :'D |
Added two parsers for SonarQube: