Skip to content
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

Improve Pylint parser #73

Merged
merged 5 commits into from
Jan 15, 2019
Merged

Improve Pylint parser #73

merged 5 commits into from
Jan 15, 2019

Conversation

bpedersen2
Copy link
Contributor

Adopt the parser to the standard 'parseable' output of pylint and use the symbolic message descriptor
instead of the numeric one.

This is a part of solving JENKINS-44915 and JENKINS-55340. The changes to the warnings-ng plugin will follow later.

Update regex to use Java8 named groups and adopt to the standard
pylint (>2.0)  "parseable" output format.

Adopt test accordingly and supply the python source for generating
the output here as well.
This allows to adjust the parser to custom output formats.
@codecov
Copy link

codecov bot commented Dec 28, 2018

Codecov Report

Merging #73 into master will decrease coverage by 0.13%.
The diff coverage is 61.53%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #73      +/-   ##
============================================
- Coverage     87.01%   86.87%   -0.14%     
  Complexity     1144     1144              
============================================
  Files           152      152              
  Lines          3650     3657       +7     
  Branches        385      387       +2     
============================================
+ Hits           3176     3177       +1     
- Misses          327      331       +4     
- Partials        147      149       +2
Impacted Files Coverage Δ Complexity Δ
...va/edu/hm/hafner/analysis/parser/PyLintParser.java 70.37% <61.53%> (-19.63%) 6 <5> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a7a4d7e...108e2f3. Read the comment docs.

@codecov
Copy link

codecov bot commented Dec 28, 2018

Codecov Report

Merging #73 into master will decrease coverage by 0.08%.
The diff coverage is 77.27%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #73      +/-   ##
============================================
- Coverage     86.87%   86.78%   -0.09%     
- Complexity     1151     1152       +1     
============================================
  Files           157      157              
  Lines          3741     3747       +6     
  Branches        406      408       +2     
============================================
+ Hits           3250     3252       +2     
- Misses          331      333       +2     
- Partials        160      162       +2
Impacted Files Coverage Δ Complexity Δ
...va/edu/hm/hafner/analysis/parser/PyLintParser.java 76.92% <77.27%> (-13.08%) 7 <6> (+1)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 18bb371...e1e7f17. Read the comment docs.

@uhafner
Copy link
Member

uhafner commented Dec 29, 2018

Are the files pymodule.py and init required?

super(DEFAULT_PYLINT_ERROR_PATTERN);
}

public PyLintParser(@CheckForNull String regExp) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason for this constructor?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be able to make the regex user-configurable.

Now used in the legacy test, the changes for warnings-ng plugin will follow later.

String message = matcher.group("message");
String category = matcher.group("symbol");
if (StringUtils.isEmpty(category)) {
category = UNKNOWN_CAT;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whitespace

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouldn't it make more sense to set the category to - in that case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer a more verbose one and pylint-specific here,

pom.xml Outdated Show resolved Hide resolved


public static final String UNKNOWN_CAT = "pylint-unknown";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

package private

@uhafner
Copy link
Member

uhafner commented Jan 13, 2019

Ping?

@bpedersen2
Copy link
Contributor Author

I am back to work now and try to update this the next days.

@uhafner
Copy link
Member

uhafner commented Jan 14, 2019

Are the files pymodule.py and init required?

@uhafner
Copy link
Member

uhafner commented Jan 14, 2019

Hasn't the new format just a new additional (optional) category? So we can have one parser without a parameter to parse the old and the new format? So the LegacyTestCase would use the same parser, just some matcher groups are empty. Or is that not possible? How would a user choose the correct parser in the UI with your approach? Typically the user selects a parser and the parser is responsible to parse the format of different versions.

@bpedersen2
Copy link
Contributor Author

Hasn't the new format just a new additional (optional) category? So we can have one parser without a parameter to parse the old and the new format? So the LegacyTestCase would use the same parser, just some matcher groups are empty. Or is that not possible? How would a user choose the correct parser in the UI with your approach? Typically the user selects a parser and the parser is responsible to parse the format of different versions.

No, the order of the fields is quite different (and it is freely user-configurable on the pylint side). The idea is to supply a default that parses the 'default' (== 'parseable') output of pylint and allow the user to supply a suitable regex for other formats.

Actually, I have never seen a pylint version in the last 5 years that produces anything resembling the old testcase ( especially the 'C' only is somthing that seems impossible to generate, as the msg_id is always [CRWE][0-9]{4}.

@bpedersen2
Copy link
Contributor Author

Are the files pymodule.py and init required?

It depends what one considers 'required' . They are not required from a java code point of view, but I think it is essential to have standard files to regenerate the test input if upstream pylint changes.

@uhafner uhafner merged commit e1e7f17 into jenkinsci:master Jan 15, 2019
@uhafner
Copy link
Member

uhafner commented Jan 15, 2019

Ok, thanks! I simplified the parser a little bit and remove the regexp in the API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants