-
Notifications
You must be signed in to change notification settings - Fork 183
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
Pylint: Add types and make categories types and categories, categories #104
Pylint: Add types and make categories types and categories, categories #104
Conversation
|
||
private static final String UNKNOWN_CAT = "pylint-unknown"; | ||
private static final String UNKNOWN_CAT = "pylint-unknown-category"; | ||
private static final String UNKNOWN_TYPE = "pylint-unkown-type"; |
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.
Typo in pylint-unknown-type
. I'm missing a n
after the k
.
Codecov Report
@@ Coverage Diff @@
## master #104 +/- ##
============================================
+ Coverage 86.48% 86.63% +0.14%
- Complexity 1192 1203 +11
============================================
Files 159 159
Lines 3840 3852 +12
Branches 428 429 +1
============================================
+ Hits 3321 3337 +16
+ Misses 357 355 -2
+ Partials 162 160 -2
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #104 +/- ##
============================================
- Coverage 86.45% 86.39% -0.07%
- Complexity 1190 1196 +6
============================================
Files 159 159
Lines 3832 3843 +11
Branches 426 427 +1
============================================
+ Hits 3313 3320 +7
- Misses 357 359 +2
- Partials 162 164 +2
Continue to review full report at Codecov.
|
@@ -39,14 +40,38 @@ protected boolean isLineInteresting(final String line) { | |||
protected Optional<Issue> createIssue(final Matcher matcher, final IssueBuilder builder) { | |||
String category = matcher.group("category"); | |||
builder.setSeverity(mapPriority(category)); | |||
builder.setCategory(StringUtils.firstNonBlank(matcher.group("symbol"), category, UNKNOWN_CAT)); | |||
builder.setCategory(StringUtils.firstNonBlank(mapCategory(category), UNKNOWN_CAT)); |
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.
Since mapCategory
is non-empty,
builder.setCategory(mapCategory(category));
|
||
return builder.setFileName(matcher.group("path")) | ||
.setLineStart(matcher.group("line")) | ||
.setMessage(matcher.group("message")) | ||
.buildOptional(); | ||
} | ||
|
||
private String mapCategory(final String category) { | ||
if (category.isEmpty()) { |
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.
StringUtils.isEmpty(category)
, otherwise a NPE might occur
case 'E': | ||
case 'F': | ||
case "E": | ||
case "F": | ||
return Severity.WARNING_HIGH; |
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 also have Severity.ERROR
. Would it make sense to use it as well?
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 is a really good idea.
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.
Done
4b41e9a
to
a2b1205
Compare
@@ -19,9 +19,10 @@ | |||
private static final long serialVersionUID = 4464053085862883240L; | |||
|
|||
// the default pattern matches "--output-format=parseable" output. | |||
private static final String PYLINT_PATTERN = "(?<path>[^:]*)(?:\\:(?<module>.*))?:(?<line>\\d+): \\[(?<category>\\D\\d*)(?:\\((?<symbol>.*)\\), )?.*?\\] (?<message>.*)"; | |||
private static final String PYLINT_PATTERN = "(?<path>[^:]*)(?:\\:(?<module>.*))?:(?<line>\\d+): \\[(?<type>(?<category>[A-Z])\\d*)?(?:\\((?<symbol>.*)\\), )?.*?\\] (?<message>.*)"; |
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 type (and so the category) group is now optional. That way, in the case where someone would not add {msg_id}
in the --msg-template
, we won't have a java.util.NoSuchElementException
error.
private Severity mapPriority(final String category) { | ||
// First letter of the Pylint classification is one of F/E/W/R/C. E/F/W are high | ||
// priority. | ||
|
||
// See http://docs.pylint.org/output.html for definitions of the categories | ||
if (category.isEmpty()) { | ||
if (StringUtils.isEmpty(category)) { |
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 also added StringUtils.isEmpty
to protect against NPE.
Thanks! |
As mentionned in Jira, the actual categories for Pylint should be types.
So I took the time to do the change. So I swapped the categories for the types, and introduced real categories. Now the Pylint parser does the same thing as the other parsers.
Note that I also added the category
[I]nformational
since it was missing and it's a valid Pylint category.