Fix/lint pr annotations #14059
Fix/lint pr annotations #14059
Conversation
|
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
|
You can test the changes on this Pull Request by downloading the APK here. |
There was a problem hiding this comment.
👋 @loremattei !
I have reviewed this PR as per the instructions, everything looks good, great job! 🌟 🌟
I have left one question (❓) for you to consider. Can we discuss that before merging this PR to develop?
WordPress/build.gradle
Outdated
| @@ -442,7 +442,8 @@ task violationCommentsToGitHub(type: se.bjurr.violations.comments.github.plugin. | |||
| {{violation.message}} | |||
| """ | |||
| violations = [ | |||
| ["CHECKSTYLE", ".", ".*/build/.*\\.xml\$", "Checkstyle"] | |||
| ["CHECKSTYLE", ".", ".*/build/.*\\.xml\$", "Checkstyle"], | |||
There was a problem hiding this comment.
Question (❓): Wouldn't it better if we update the CheckStyle configuration to a less generic path, just like we do for Detekt?
For example: ["CHECKSTYLE", ".", ".*/build/.*/checkstyle/.*\\.xml\$", "CheckStyle"]
I am actually not sure how this path is being traversed by the Violation Comments plugin, but I worry that this regular expression might resolve into multiple paths. I speculate that the reason it works atm might be that even though this regular expression resolves into multiple paths, the first result is being taken into consideration. And, since the first result ends up being the checkstyle.xml file it works. But, this might break in the future. Wdyt?
There was a problem hiding this comment.
Good point!
I've pushed a commit to update the filter.
There was a problem hiding this comment.
Great @loremattei , many thanks!
I reviewed this commit and it LGTM, I will now approve and merge this PR.
This PR fixes #14051 by upgrading the Violations plugin.
I also updated the GH token the plugin uses in order to consolidate across the various tools (for reasons, we have two different token used, which is confusing) and configured it to annotate the violations found by Detekt as well.
To test:
PR submission checklist:
RELEASE-NOTES.txtif necessary.