Checkstyle Plugin Cleanup (#3520)

* 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

build.mill

@@ -736,6 +736,7 @@ object dist0 extends MillPublishJavaModule {
     build.contrib.playlib.testDep(),
     build.contrib.playlib.worker("2.8").testDep(),
     build.contrib.errorprone.testDep(),
+    build.contrib.checkstyle.testDep(),
     build.bsp.worker.testDep(),
     build.testkit.testDep()
   )

contrib/checkstyle/src/mill/contrib/checkstyle/CheckstyleArgs.scala

@@ -11,8 +11,8 @@ import mainargs.{Leftover, ParserForClass, main}
  */
 @main(doc = "arguments for CheckstyleModule.checkstyle")
 case class CheckstyleArgs(
-    check: Boolean = false,
-    stdout: Boolean = false,
+    check: Boolean = true,
+    stdout: Boolean = true,
     sources: Leftover[String]
 )
 object CheckstyleArgs {

contrib/checkstyle/src/mill/contrib/checkstyle/CheckstyleModule.scala

@@ -18,8 +18,12 @@ trait CheckstyleModule extends JavaModule {
    * @note [[sources]] are processed when no [[CheckstyleArgs.sources]] are specified.
    */
   def checkstyle(@mainargs.arg checkstyleArgs: CheckstyleArgs): Command[Int] = T.command {
+    val (output, exitCode) = checkstyle0(checkstyleArgs.stdout, checkstyleArgs.sources)()
 
-    val CheckstyleArgs(check, stdout, leftover) = checkstyleArgs
+    checkstyleHandleErrors(checkstyleArgs.stdout, checkstyleArgs.check, exitCode, output)
+  }
+
+  protected def checkstyle0(stdout: Boolean, leftover: mainargs.Leftover[String]) = T.task {
 
     val output = checkstyleOutput().path
     val args = checkstyleOptions() ++
@@ -36,17 +40,27 @@ trait CheckstyleModule extends JavaModule {
       classPath = checkstyleClasspath().map(_.path),
       mainArgs = args,
       workingDir = millSourcePath, // allow passing relative paths for sources like src/a/b
+      streamOut = true,
       check = false
     ).exitCode
 
+    (output, exitCode)
+  }
+
+  protected def checkstyleHandleErrors(
+      stdout: Boolean,
+      check: Boolean,
+      exitCode: Int,
+      output: os.Path
+  )(implicit ctx: mill.api.Ctx): Int = {
+
     val reported = os.exists(output)
     if (reported) {
       T.log.info(s"checkstyle output report at $output")
     }
 
-    if (exitCode == 0) {
-      T.log.info("checkstyle found no violation")
-    } else if (exitCode < 0 || !(reported || stdout)) {
+    if (exitCode == 0) {} // do nothing
+    else if (exitCode < 0 || !(reported || stdout)) {
       T.log.error(
         s"checkstyle exit($exitCode); please check command arguments, plugin settings or try with another version"
       )
@@ -73,7 +87,7 @@ trait CheckstyleModule extends JavaModule {
    * Checkstyle configuration file. Defaults to `checkstyle-config.xml`.
    */
   def checkstyleConfig: T[PathRef] = T {
-    PathRef(millSourcePath / "checkstyle-config.xml")
+    PathRef(T.workspace / "checkstyle-config.xml")
   }
 
   /**

contrib/checkstyle/src/mill/contrib/checkstyle/CheckstyleXsltModule.scala

@@ -13,7 +13,8 @@ trait CheckstyleXsltModule extends CheckstyleModule {
    * Runs [[CheckstyleModule.checkstyle]] and uses [[CheckstyleModule.checkstyleOutput]] to generate [[checkstyleXsltReports]].
    */
   override def checkstyle(@mainargs.arg checkstyleArgs: CheckstyleArgs): Command[Int] = T.command {
-    val numViolations = super.checkstyle(checkstyleArgs)()
+    val (output, exitCode) = checkstyle0(false, checkstyleArgs.sources)()
+
     val checkOutput = checkstyleOutput().path
 
     if (os.exists(checkOutput)) {
@@ -38,15 +39,13 @@ trait CheckstyleXsltModule extends CheckstyleModule {
       }
     }
 
-    numViolations
+    checkstyleHandleErrors(checkstyleArgs.stdout, checkstyleArgs.check, exitCode, output)
   }
 
   /**
-   * `xml`
+   * Necessary in order to allow XSLT transformations on the results
    */
-  final override def checkstyleFormat: T[String] = T {
-    "xml"
-  }
+  final override def checkstyleFormat: T[String] = "xml"
 
   /**
    * Set of [[CheckstyleXsltReport]]s.

contrib/checkstyle/test/src/mill/contrib/checkstyle/CheckstyleModuleTest.scala

@@ -182,7 +182,6 @@ object CheckstyleModuleTest extends TestSuite {
       args: CheckstyleArgs
   ): Boolean = {
     val eval = UnitTester(module, modulePath)
-
     eval(module.checkstyle(args)).fold(
       {
         case api.Result.Exception(cause, _) => throw cause
@@ -194,12 +193,12 @@ object CheckstyleModuleTest extends TestSuite {
 
           val Right(report) = eval(module.checkstyleOutput)
 
-          if (os.exists(report.value.path))
+          if (os.exists(report.value.path)) {
             violations.isEmpty || {
               val lines = os.read.lines(report.value.path)
               violations.forall(violation => lines.exists(_.contains(violation)))
             }
-          else
+          } else
             args.stdout
         }
       }

contrib/checkstyle/test/src/mill/contrib/checkstyle/CheckstyleXsltModuleTest.scala

@@ -47,7 +47,7 @@ object CheckstyleXsltModuleTest extends TestSuite {
   def testModule(module: TestBaseModule with CheckstyleXsltModule, modulePath: os.Path): Boolean = {
     val eval = UnitTester(module, modulePath)
 
-    eval(module.checkstyle(CheckstyleArgs(sources = Leftover()))).fold(
+    eval(module.checkstyle(CheckstyleArgs(check = false, sources = Leftover()))).fold(
       {
         case api.Result.Exception(cause, _) => throw cause
         case failure => throw failure

docs/modules/ROOT/nav.adoc

@@ -9,6 +9,7 @@
 * xref:Java_Module_Config.adoc[]
 * xref:Java_Build_Examples.adoc[]
 * xref:Testing_Java_Projects.adoc[]
+* xref:Linting_Java_Projects.adoc[]
 // * xref:Publishing_Java_Projects.adoc[]
 * xref:Java_Web_Examples.adoc[]
 * xref:Case_Study_Mill_vs_Maven.adoc[]
@@ -56,6 +57,7 @@
 ** xref:contrib/codeartifact.adoc[]
 ** xref:contrib/docker.adoc[]
 ** xref:contrib/errorprone.adoc[]
+** xref:contrib/checkstyle.adoc[]
 ** xref:contrib/flyway.adoc[]
 ** xref:contrib/gitlab.adoc[]
 ** xref:contrib/jmh.adoc[]

docs/modules/ROOT/pages/Contrib_Plugins.adoc

@@ -39,6 +39,7 @@ import $ivy.`com.lihaoyi::mill-contrib-bloop:`
 * xref:contrib/codeartifact.adoc[]
 * xref:contrib/docker.adoc[]
 * xref:contrib/errorprone.adoc[]
+* xref:contrib/checkstyle.adoc[]
 * xref:contrib/flyway.adoc[]
 * xref:contrib/gitlab.adoc[]
 * xref:contrib/jmh.adoc[]

docs/modules/ROOT/pages/Java_Module_Config.adoc

@@ -88,6 +88,3 @@ If you are using millw, a more permanent solution could be to set the environmen
 
 include::example/javalib/module/13-jni.adoc[]
 
-== Using the ErrorProne plugin to detect code problems
-
-include::example/javalib/module/14-error-prone.adoc[]

docs/modules/ROOT/pages/Linting_Java_Projects.adoc

@@ -0,0 +1,24 @@
+= Linting Java Projects
+
+++++
+<script>
+gtag('config', 'AW-16649289906');
+</script>
+++++
+
+This page will discuss common topics around working with test suites using the Mill build tool
+
+== ErrorProne
+
+include::example/javalib/linting/1-error-prone.adoc[]
+
+== Checkstyle
+
+include::example/javalib/linting/2-checkstyle.adoc[]
+
+== Jacoco Code Coverage
+
+Mill supports Java code coverage analysis via the mill-jacoco plugin. See the
+plugin repository documentation for more details:
+
+* https://github.com/lefou/mill-jacoco

example/javalib/linting/2-checkstyle/build.mill

@@ -0,0 +1,37 @@
+package build
+import mill._, javalib._
+import $ivy.`com.lihaoyi::mill-contrib-checkstyle:`
+
+import mill.contrib.checkstyle._
+
+object `package` extends RootModule with CheckstyleModule {
+  def checkstyleVersion = "9.3"
+}
+
+/** Usage
+
+> ./mill checkstyle # run checkstyle to produce a report, defaults to warning without error
+...src/InputWhitespaceCharacters.java:3:23: Line contains a tab character...
+...src/InputWhitespaceCharacters.java:16:3: Line contains a tab character...
+...src/InputFileName1.java:2:1: Top-level class MyAnnotation1 has to reside in its own source file...
+...src/InputFileName1.java:13:1: Top-level class Enum1 has to reside in its own source file...
+...src/InputFileName1.java:26:1: Top-level class TestRequireThisEnum has to reside in its own source file...
+Audit done.
+
+> sed -i.bak 's/warning/error/g' checkstyle-config.xml # make checkstyle error on violations
+
+> ./mill checkstyle
+error: ...src/InputWhitespaceCharacters.java:3:23: Line contains a tab character...
+...src/InputWhitespaceCharacters.java:16:3: Line contains a tab character...
+...src/InputFileName1.java:2:1: Top-level class MyAnnotation1 has to reside in its own source file...
+...src/InputFileName1.java:13:1: Top-level class Enum1 has to reside in its own source file...
+...src/InputFileName1.java:26:1: Top-level class TestRequireThisEnum has to reside in its own source file...
+Audit done.
+
+> sed -i.bak 's/\t/    /g' src/InputWhitespaceCharacters.java
+
+> rm src/InputFileName1.java
+
+> ./mill checkstyle # after fixing the violations, checkstyle no longer errors
+Audit done.
+*/