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

Add support for parsing cargo check JSON output #137

Merged
merged 3 commits into from
Apr 2, 2019

Conversation

garyttierney
Copy link
Contributor

Adds a parser that can read compiler messages emitted by rustc and clippy in
the format used by cargo's --message-format json option.

This allows producing analysis reports for Rust projects using either the standard cargo check functionality, or the clippy tool.

Also, I think I need to add the parser definition to the Jenkins warnings-ng-plugin too. Should I send a PR for that now or wait until this new parser is available in the latest snapshot of analysis-model?

@codecov
Copy link

codecov bot commented Mar 31, 2019

Codecov Report

Merging #137 into master will increase coverage by 0.06%.
The diff coverage is 93.47%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #137      +/-   ##
============================================
+ Coverage     87.89%   87.95%   +0.06%     
- Complexity     1211     1218       +7     
============================================
  Files           157      158       +1     
  Lines          3865     3911      +46     
  Branches        424      427       +3     
============================================
+ Hits           3397     3440      +43     
- Misses          311      312       +1     
- Partials        157      159       +2
Impacted Files Coverage Δ Complexity Δ
...du/hm/hafner/analysis/parser/CargoCheckParser.java 93.47% <93.47%> (ø) 7 <7> (?)
...ava/edu/hm/hafner/analysis/parser/JavacParser.java 100% <0%> (ø) 6% <0%> (ø) ⬇️

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 8dea934...6768bd3. Read the comment docs.

@codecov
Copy link

codecov bot commented Mar 31, 2019

Codecov Report

Merging #137 into master will increase coverage by 0.06%.
The diff coverage is 93.47%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #137      +/-   ##
============================================
+ Coverage     87.89%   87.95%   +0.06%     
- Complexity     1211     1218       +7     
============================================
  Files           157      158       +1     
  Lines          3865     3911      +46     
  Branches        424      427       +3     
============================================
+ Hits           3397     3440      +43     
- Misses          311      312       +1     
- Partials        157      159       +2
Impacted Files Coverage Δ Complexity Δ
...du/hm/hafner/analysis/parser/CargoCheckParser.java 93.47% <93.47%> (ø) 7 <7> (?)

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 bf854ef...7bd6ad0. Read the comment docs.

Copy link
Member

@uhafner uhafner left a comment

Choose a reason for hiding this comment

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

Thanks for your PR. The code is very well written! You still can improve the code coverage if you add one or more two test cases, see comments.

I think it makes sense to wait with the associated change in warnings-ng until I publish a snapshot release of your changes.


/**
* A parser for {@code rustc} compiler messages in the JSON format emitted by {@code cargo check --message-format json}.
* .
Copy link
Member

Choose a reason for hiding this comment

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

Duplicate .

If you like please add @author

}
}

return Optional.empty();
Copy link
Member

Choose a reason for hiding this comment

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

Any chance to write a test that hits this line?

for (int index = 0; index < spans.length(); index++) {
JSONObject span = spans.getJSONObject(index);

if (span.getBoolean(MESSAGE_SPAN_IS_PRIMARY)) {
Copy link
Member

Choose a reason for hiding this comment

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

Any chance to write a test that skips this line once? I.e. that loops for two times?


/**
* Tests the class {@link CargoCheckParser}.
*/
Copy link
Member

Choose a reason for hiding this comment

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

@author

.hasColumnStart(5)
.hasColumnEnd(34);
}

Copy link
Member

Choose a reason for hiding this comment

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

It would be helpful to have an additional test that also checks the aggregation, i.e. has a size of 2 issues.

@uhafner
Copy link
Member

uhafner commented Apr 1, 2019

Can you please also add a line to the ChangeLog?

@garyttierney
Copy link
Contributor Author

Thanks for the review! I'll action these changes a bit later today.

Adds a parser that can read compiler messages emitted by rustc and clippy in
the format used by cargo's `--message-format json` option.
Also updates some javadoc comments to add author tags and remove erroneous
punctuation.
@uhafner
Copy link
Member

uhafner commented Apr 2, 2019

Thanks! I'll ping you if the release (or snapshot) is published so we can use it in warnings-ng.

@uhafner uhafner merged commit e13d1a8 into jenkinsci:master Apr 2, 2019
@garyttierney garyttierney deleted the feature/cargo-check-parser branch April 2, 2019 11:59
@uhafner
Copy link
Member

uhafner commented Apr 26, 2019

If you want to change something or add icon, help, etc, please file a PR to warning-ng:

jenkinsci/warnings-ng-plugin@fcf33e4

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