-
Notifications
You must be signed in to change notification settings - Fork 280
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
Use category if type is empty when publishing checks #1317
Use category if type is empty when publishing checks #1317
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1317 +/- ##
============================================
- Coverage 79.68% 79.68% -0.01%
- Complexity 1444 1446 +2
============================================
Files 249 249
Lines 5528 5532 +4
Branches 425 426 +1
============================================
+ Hits 4405 4408 +3
Misses 974 974
- Partials 149 150 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. |
57422ba
to
a911927
Compare
plugin/src/test/java/io/jenkins/plugins/analysis/core/steps/WarningChecksPublisherITest.java
Outdated
Show resolved
Hide resolved
ChecksDetails details = publisher.extractChecksDetails(AnnotationScope.PUBLISH_NEW_ISSUES); | ||
|
||
assertThat(details.getOutput().get().getChecksAnnotations().get(0)) | ||
.hasFieldOrPropertyWithValue("title", Optional.of("C4101")); |
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.
C4101
seems to be a strange category, shouldn't this be the type actually? Maybe that is the root cause...
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.
Yes as I written in the description most of the MsBuild warnings have no type and only a category. The MsBuild parser sets the category instead of the type, when no category is provided. But changing the MsBuild parser would be a breaking change, or would you prefer to fix the issue inside the MsBuld parser?
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 you change here makes sense anyway as some other parsers might have an empty type as well.
But in general it would also make sense to reconsider the existing implementation of the MsBuild parser and see if the mapping can be improved. I'm using it not on my own so I am not sure if such a change would be helpful.
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.
Issue category and type are both shown inside the "issue view". The other use case I am aware of is ignoring / filtering issues, where its only a mater of configuration. But at least its a little confusing.
a911927
to
f4e6cd7
Compare
f4e6cd7
to
56b2ee3
Compare
plugin/src/test/java/io/jenkins/plugins/analysis/core/steps/WarningChecksPublisherITest.java
Outdated
Show resolved
Hide resolved
…rningChecksPublisherITest.java Fix CheckStyle suppression.
When using ChecksPublisher to publish warnings generated by MsBuild, the annotations title is mostly empty (
-
) because MsBuild parser creates issues with only acategory
when it can't determine both the issuecategory
and the issuetype
.So the real problem is that the MsBuild parser misuses the issue 'category' but I think changing it would be a brackig change (e.g. breaks issue filters) and only changing the 'visualization' is the 'safer' way. And eventually there are other parsers that have the same 'problem' and this way the user gets more information about the issue.