hazendaz commented on a change in pull request #28: [MCHECKSTYLE-387] emit a
warning when using an old version of checkstyle
URL:
https://github.com/apache/maven-checkstyle-plugin/pull/28#discussion_r386163906
##########
File path:
src/main/java/org/apache/maven/plugins/checkstyle/exec/DefaultCheckstyleExecutor.java
##########
@@ -314,6 +314,13 @@ public URLClassLoader run()
try
{
checker.setClassLoader( projectClassLoader );
+ /*
+ * MCHECKSTYLE-387: If the previous method call was successful,
emit a warning that the user is using
+ * an old version of checkstyle.
+ */
+ getLogger().warn( "Old version of checkstyle detected. Consider
updating to >= v8.30" );
+ getLogger().warn( "For more information see: "
+ +
"https://maven.apache.org/plugins/maven-checkstyle-plugin/examples/upgrading-checkstyle.html";
);
}
catch ( NoSuchMethodError ignored )
Review comment:
@rnveach Thanks I saw that but missed the important parts. So really, the
issue is not that the maven plugin doesn't handle properly at the lines this PR
attempts to solve but rather subsequently on line 204 of the file. A check
needs added so this gracefully exists without failing at all. I find this
plugin execution optional in most ci/cd pipelines and it is horrible that it
even fails hard like this. The fix this PR does though is still not in the
appropriate place. Maven knows the plugins used, the versions used, etc. It
could check at start of the run and always warn that latest version it knows
about is not being used. It should then subsequently more gracefully handle
failures. The output here itself is good, but really just too far into the
process when it could have been reported earlier. In fact, it could even be
set to skip run entirely if the version doesn't match the plugin and then
actually start releasing this plugin to target checkstyle releases like it
should be to start with.
Overall does this PR itself help, I guess, it will still fail so useful
information will be present that helps general user but it just feels wrong and
is allowing the problems to persist with this plugin in general instead of
fixing the real issue.
Maybe a better conversation would be if maven should even be the owners of
this plugin to start with. Seems to me it might get steered better if it were
under guidence of checkstyle team (proviced expertice with maven bits is
there). This is of course off topic a bit but worth expressing.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
