Add Spotless plugin support

[ayewo]

This PR adds official support in Mill for a SpotlessModule via the addition of 3 Spotless traits:

• JavaSpotlessModule
• KotlinSpotlessModule and
• ScalaSpotlessModule

which can be used with JavaModule, KotlinModule and ScalaModule respectively. This is because each of those 3 JVM languages have different 3rd-party dependencies.

It works similar in style to PalantirFormatModule.

This is intended to close #3888

[lihaoyi]

Is there some way to configure whether Spotless uses ktfmt or ktlint? AFAIK both of them provide formatters. Or is Spotless hardcoded to use one or the other?

[ayewo]

There's is currently no way to configure whether Spotless uses ktlint. It's currently set as the default.

If a valid config for ktfmt is provided via KotlinConfig(...) in the build.mill, then ktfmt will also be used, alongside the default formatter i.e. ktlint.

You want both of them to be configurable?

[lihaoyi]

No need then, I think we can just follow the upstream convention

[ayewo]

Based on your answer, it sounds like you were asking about upstream Spotless, not my implementation?
My answer was regarding my implementation.

If you were specifically asking regarding the Spotless project, this is how they do it for Kotlin:

  kotlin {
    // by default the target is every '.kt' and '.kts` file in the java sourcesets
    ktfmt()    // has its own section below
    ktlint()   // has its own section below
    diktat()   // has its own section below
    prettier() // has its own section below
    licenseHeader '/* (C)$YEAR */' // or licenseHeaderFile
  }

In the example build.gradle file above, the presence of ktfmt() would correspond to the invocation of the default version of ktfmt via this config method ktfmt().

  kotlin {
    ktfmt("0.51") 
    ...
  }

In the example above, passing a specific version i.e. ktfmt("0.51") would invoke a similar config method that takes a version argument: ktfmt(String version).

  kotlin {
    licenseHeader '/* (C)$YEAR */' // or licenseHeaderFile
  }

In this example, ktfmt will be skipped entirely since it is completely absent from the steps.

[ayewo]

Following the upstream convention would mean refactoring KotlinConfig and similar case classes to look like this:

case class KotlinConfig(
    target: String = ".kt",
    licenseHeader: Option[String] = None,
    licenseHeaderFile: Option[String] = None,
    override val licenseHeaderDelimiter: String = "(package |@file|import )",
    ktfmt: Boolean = false,
    ktfmtVersion: String = "0.53",
    ktfmtOptions: Option[KtfmtOptions] = None,
    klint: Boolean = false,
    klintVersion: String = "1.5.0"
) extends JVMLangConfig {
  require(
    !(licenseHeader.isDefined && licenseHeaderFile.isDefined),
    "Please specify only licenseHeader or licenseHeaderFile but not both"
  )    

  def ktfmt(): KotlinConfig =
    copy(ktfmt = true)

  def ktfmt(version: String): KotlinConfig =
    copy(ktfmt = true, ktfmtVersion = version)

  def ktfmtOptions(options: KtfmtOptions): KotlinConfig =
    copy(ktfmtOptions = Some(options))

  def ktlint(): KotlinConfig =
    copy(klint = true)

  def ktlint(version: String): KotlinConfig =
    copy(klint = true, klintVersion = version)
...
}

With this change, ktlint would no longer added as a formatter step by default.

[lihaoyi]

I think the config looks fine as is. Let's drop the with updaters as mentioned earlier, then we can merge it

[lihaoyi]

Is there a reason you want to use forwarders rather than using the .copy method directly?

[ayewo]

No reason other than I thought you wanted it to be similar to the style of Spotless in Groovy.

[lihaoyi]

Let's use the .copy methods directly then. We need to find a balance between the upstream style and the local Scala style, and in this case I think .copy wins out

[ayewo]

I've updated to use .copy() directly now.

[ayewo]

@lihaoyi Anything else left for this to be merged ?

[lihaoyi]

We should not need to use .copy here at all I think, we can just use the GoogleJavaFormat(...) constructor. Same as the others?

[ayewo]

Removed as requested.

[lefou]

I'd prefer to have the top-level classes in their own files.

[ayewo]

@lefou Done.

[ayewo]

I believe I've fixed all instances of versions being hard-coded.

This is ready to be merged @lihaoyi.

[lihaoyi]

@ayewo the example test seems to be failing, you can find instructions to run it locally in the contributing.adoc. The example tests serve as both tests and documentation, so you need to make sure the tests pass and the english explanations are properly written up and included in website/docs/modules/ROOT/pages/javalib/linting.adoc, website/docs/modules/ROOT/pages/scalalib/linting.adoc, website/docs/modules/ROOT/pages/kotlinlib/linting.adoc. See the other example tests and their place in the documentation for reference

[lefou]

It might have slipped thru in previous reviews, but it looks like you expecting the spotless library on the build classpath, whereas what we want is the SpotlessModule to load all necessary libraries dynamically on demand. The whole point of abstracting over the spotless API is, that spotless itself is not on the classpath and will only be loaded dynamically based on the version configured in the module. All code that is coded against any spotless API needs therefore to reside in a dedicated module.

For example, ZincWorkerModule is resposible to abstract over a concrete zinc dependency. The shared API (ZinkWorkerApi) is loaded from the mill.scalalib.api module, whereass the implemntation that has access to the actual zinc API resides in mill.scalalib.worker and is loaded on demand by ZincWorkerModule.

[lefou]

I don't think this comment is correct

[ayewo]

It might have slipped thru in previous reviews, but it looks like you expecting the spotless library on the build classpath, whereas what we want is the SpotlessModule to load all necessary libraries dynamically on demand. The whole point of abstracting over the spotless API is, that spotless itself is not on the classpath and will only be loaded dynamically based on the version configured in the module. All code that is coded against any spotless API needs therefore to reside in a dedicated module.

For example, ZincWorkerModule is resposible to abstract over a concrete zinc dependency. The shared API (ZinkWorkerApi) is loaded from the mill.scalalib.api module, whereass the implemntation that has access to the actual zinc API resides in mill.scalalib.worker and is loaded on demand by ZincWorkerModule.

I don't understand what you are saying here.

The main module doing all the heavy-lifting is SpotlessModule and it already depends on CoursierModule:

trait SpotlessModule extends CoursierModule

[lefou]

Let me point out some issues or indicators, that we load or are required to load the spotless API on the Mill classpath. Beside the fact that SpotlessModule extends CoursierModule, all com.diffplug.spotless._ imports don't get loaded via that CoursierModule but are rather already on the classpath, as you added spotless to the dependencies of mill.scalalib module.

[lefou]

We should not required to add spotless-lib to the Mill classpath.

[lefou]

This import means, the spotless API is already on the Mill classpath.

[lefou]

These imports mean, we need spotless on the Mill classpath.

[lefou]

This dependency forced spotless on the Mill classpath.

[ayewo]

@lefou
To add, I don't understand how you can say that? Li clearly stated in the original ticket: #3888 that

Should be similar in style of PalantirFormatModule, ScalafmtModule, ErrorProneModule, etc.

Currently putting together the documentation changes that Li requested. I finished the core of this ticket more than 2 weeks ago. The only thing keeping this from being merged are nits that were pointed out during review.

It's not nice at all, to point out nits, a contributor fixes them, the cycle repeats, then you come swinging pointing out a fresh set of nits. Please be respectful of contributors time.

[lefou]

It's not nice at all, to point out nits, a contributor fixes them, the cycle repeats, then you come swinging pointing out a fresh set of nits. Please be respectful of contributors time.

@ayewo I'm sorry if you feel that way. And I understand your emotions under the assumptions, these are nits. But those points are not nits. And it's not only your time that is spent here. Until now I only commented on smaller parts under the assumption that the main feature was already reviewed by @lihaoyi. Now, I recognized some major issues and felt the need to point them out.

I understand, that the amount of work to finish this PR is likely higher compared to e.g. implementing PalantirModule, but my review is not bound to any assumptions you might have based on the amount of a bounty.

Just to make it clear, my latest review points are not nits. Your PR description claims to do it the same way as other modules, yet all those other modules don't add any dependencies to mill.scalalib. That does not mean the general design and all review that went into it, is lost work. It only means, we need to move some of these API classes into dedicated modules and load them at runtime.

I'm glad to help you with the module split-up, if you need help. My main focus is to get a new feature into Mill without breaking all other properties and users of Mill.

[lihaoyi]

@lefou's is correct, the issues he raised are important, I missed them in my own review, and we should resolve them before merging this. I apologize for not being thorough enough in my own review. @lefou has offered to help get it in if you would like it.

Some friction in getting PRs in is expected. If we were all sitting in the same room this would be smoother, but we're scattered around the world, so we'll have to live with some inefficiency.