Add Checkstyle plugin #3472
Add Checkstyle plugin #3472
Conversation
|
Build failures look like test infra flakiness/races (not finding the just-published module for the integration test, and a concurrent file opening error on Windows) rather than real issues? |
|
Will take a look. Our test suite is flaky, I've been working on it but not 100% stabilized yet |
|
Manually retries those jobs |
| * | ||
| * @author Andrew Johnson | ||
| */ | ||
| case class CheckstyleXSLTSettings(xslt: File, output: File) |
There was a problem hiding this comment.
Do we actually need an output: File for each XSLT transformation? If we're always applying the same transformations in the same sequence on the input checkstyle config between using it, the choice of output file name seems irrelevant and seems like something Mill can manage
There was a problem hiding this comment.
I think the idea is that each transformation has its own output file (e.g. one transformation for a human-readable HTML page and one for some kind of internal XML report). To be honest it's not a feature I've used, just trying to retain parity with the SBT plugin.
| } | ||
|
|
||
| /** An optional set of XSLT transformations to be applied to the checkstyle output */ | ||
| def checkstyleXsltTransformations: T[Option[Set[CheckstyleXSLTSettings]]] = T.input { None } |
There was a problem hiding this comment.
This type signature here looks a bit off. Do we need it to be wrapped in Option, when it could be a Set.empty? If transforms are to be run sequentially, and the ordering matters, should it be a Seq instead? Should we provide a default path to put the file, or default folder to scan for such files, like how we look for T.workspace / "checkstyle-config.xml" above?
There was a problem hiding this comment.
Agreed that there's no point in the Option wrapping, will fix that.
I think this is a pretty niche feature so probably the default should be no such file.
| sealed trait CheckstyleConfigLocation { | ||
| def read(resources: Seq[Path]): String | ||
| } |
There was a problem hiding this comment.
Could this trait hierarchy be simplified? Classpath seems strange, since it's loading stuff from the Mill classloader, which won't contain any of the application classpath.
URL seems redundant, since if we ask for a file, Mill can convert that into a URL by doing override def foo = T{ requests.get("...") }, so I don't think we need to have a special case for it in the configuration ADT.
Given that, it might be best to just make the config a PathRef, and led anyone who wants to fetch that file from unusual locations just do an override the task to call requests.get or os.read(os.resource / ...) themselves
There was a problem hiding this comment.
Classpath uses the application's dependencies (if you comment out the dependency in the checkstyleClasspathConfig test it will fail). I think that's an important use case (use a config that's published in some organisational jar). Happy to cut URL if you still think that's worthwhile given that we do need the ADT?
| import mill.{Agg, T} | ||
| import mill.scalalib.{Dep, DepSyntax, JavaModule} | ||
| import mill.util.Jvm | ||
| import net.sf.saxon.s9api.Processor |
There was a problem hiding this comment.
I'm curious why we need this external dependency for Processor? What is it doing that we cannot do via scala.xml or javax.xml?
There was a problem hiding this comment.
Just took the impl from the SBT plugin, I don't have a particular preference myself.
contrib/checkstyle/src/mill/contrib/checkstyle/CheckstyleModule.scala
Outdated
Show resolved
Hide resolved
| > ./mill checkstyle # Run checkstyle to produce a report, optionally failing the build if violations are severe enough | ||
| Checkstyle warning found in ...src/InputFileName1.java:2: Top-level class MyAnnotation1 has to reside in its own source file. | ||
| Checkstyle warning found in ...src/InputFileName1.java:13: Top-level class Enum1 has to reside in its own source file. | ||
| Checkstyle warning found in ...src/InputFileName1.java:26: Top-level class TestRequireThisEnum has to reside in its own source file. | ||
| 3 issue(s) found in Checkstyle report: ...out/checkstyle.dest/checkstyle-report.xml | ||
| error: ...Checkstyle issues found |
There was a problem hiding this comment.
Let's flesh this out a bit more:
- Assert on a handful of different checkstyle errors, not just
Top-level class has to reside in its own source file - Let's exercise the process of fixing the errors: we can run
sedto mangle the files (see other example tests), and runcheckstyleagain and verify that it no longer errors
There was a problem hiding this comment.
I agree with fixing and checking it passes, have extended the test to cover that part.
What sort of different errors do we want to cover? I don't see a lot of value in duplicating the whole checkstyle checksuite, we're not really exercising the plugin code more by having more than a couple of errors (as long as we can pick up more than 1 error reported by checkstyle, I can't see any reasonable way for the plugin to break on a different error), but happy to add any you think are important.
|
@m50d I'm going to go with #3516 as the implementation, but I'll pay out 30% of the bounty for the effort you put into this. Email me your bank transfer detail and I'll wire it over. Thanks for working through this with me, and this code has definitely been useful in helping me understand the solution and design space even though we may not end up merging it |
* Set `check = true` and `stdout = true` by default since that's what I'm guessing people will expect * Extract out `def checkstyleHandleErrors` and `def checkstyle0` for re-use between `CheckstyleModule` and `CheckstyleXsltModule`. This allows `def checkstyle` to throw errors and report to console while still generating the XSLT reports in `CheckstyleXsltModule` * Avoid reporting `no violations` when exit code is zero, because there may still be warnings * Integrate the example test from @m50d's checkstyle PR #3472, flesh it out, and integrate it into the docsite
|
Thank you - glad to have contributed something and agreed that that's ultimately the better approach. |
(Commits not clean as I'm expecting to squash)
Fix: #3332