Add Reviewdog as checkstyle #6996
Conversation
* upstream/master: Update journalList.mv Update to javafx15 (#7018) Squashed 'src/main/resources/csl-styles/' changes from 6fab78b..5297abd try to fix DEP issue with official jdk (#7008) Jstor Fetcher (#6992) Group: "Searching for keywords" searches for a single keyword ==> use singular (#6995) Merge parsing of bracketed patterns (#6989) 6848 fixed the issue of clicking collapse all expanding tree (#6993) Enable auto sync per default for Open/Libre Office (#6985) Bump unirest-java from 3.11.00 to 3.11.01 (#7001) Bump byte-buddy-parent from 1.10.16 to 1.10.17 (#7004) Bump lucene-queryparser from 8.6.2 to 8.6.3 (#7002) Bump postgresql from 42.2.16 to 42.2.17 (#7005) Bump pascalgn/automerge-action from v0.11.0 to v0.12.0 (#7006) Bump flowless from 0.6.1 to 0.6.2 (#7003)
change to PR reporter
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Siedlerchr
added
the
status: ready-for-review
There was a problem hiding this comment.
I'm not a big fan that reviewdog add these warnings as comments. Github has check annotation exactly for this purpose, so I would prefer if that could be used, e.g. https://github.com/staabm/annotate-pull-request-from-checkstyle (not sure if this also works for java-checkstyle).
| @@ -0,0 +1,141 @@ | |||
| <?xml version="1.0"?> | |||
There was a problem hiding this comment.
Do we really need to duplicate the checkstyle config? Makes it hard in the future to keep them synced
There was a problem hiding this comment.
Checks are also possible aka the default, but only visible if you look at the files changed tab.
I personally think this comment variant is useful
Unfortunately we currently need this duplication because otherwise the suppression config file cannot be found due to the parameter expansion or the gradle check cannot be run.
There's also no way to inline the suppression
There was a problem hiding this comment.
It just adds so much noise, if you commit half-finished PRs that still need cleanup etc. You then get github notifications because a bot commented. Why not educate people to look at all checks?
Currently, the checkstyle issues do not show up in the changed files tab.
So with the current config, reviewdog will report about things that we actually decided to suppress?
There was a problem hiding this comment.
I changed back to github checks feature.
Again, for the copy. Reviewdog needs a copy of the checkstyle.xml because it contains the relative path to the supression.xml file. Otherwise it cannot parse the ${configloc} variable.
Gradle and the IDEs need that variable to automatically set the correct path
There was a problem hiding this comment.
Thanks!
Can you then please open an issue at reviewdog so that this gets fixed in the future. Thanks!
There was a problem hiding this comment.
It's more a checkstlye issue (that won't be fixed). However, I opened an issue at the action repo.
There was a problem hiding this comment.
The issue is nikitasavinov/checkstyle-action#16. It should have been solved with nikitasavinov/checkstyle-action#19. Should we try it?
tobiasdiez
added
the
status: changes required
tobiasdiez
removed
the
status: changes required
|
Can reviewdog also parse Junit test reports? If not it would be good to integrate something as https://github.com/marketplace/actions/junit-report-to-annotations |
|
@tobiasdiez https://github.com/reviewdog/reviewdog#input-format |
|
Ok, can you then please play around with the junit annotation action I posted above. I think this is actually more important than checkstyle. |
|
I fear the action you posted will not work on forks (due to Github acess token etc) |
Motivation: Many new contributors seem to ignore or don't see the failing github checks