GeezerVison

Huzzah!! Fireworks!!! Thanks so much @lutovich, great to finally have this off the ground! Having a functional plugin and implementation of MavenProvisioner is a huge deal! I'm going to cheat and take the easy ones first :)

• unclear how to version the plugin / publishing of the Maven plugin to Maven Central

As steps are added and improved, lib and lib-extra clunk along 1.n -> 1.n+1. For a build-specific plugin like plugin-gradle, it starts off at MAJOR.n, and each time a new lib comes out, a new plugin-gradle also comes out with a matching .n. The MAJOR is bumped to indicate that something about the integration code requires a breaking change - dropping an old version of gradle, changing the buildscript, etc. I would propose we do the same with the maven plugin. If the MAJOR ends up at 1, 2, 3, or even 15 before it stabilizes, I don't think it matters. Just as long as we're honest about when users will have to update their buildscripts.

Uploading this plugin to mavenCentral on each release will be trivial, I'll have to turn some cranks for the initial release but it'll be automatic from there.

• dependency information is duplicated in pom.xml and build.gradle. pom.xml can be generated with all dependencies from build.gradle

Do maven plugins require that they have no dependencies? It seems that if we remove the shading, the build will get a lot simpler. The gradle project has the stableLib property, which is the version of the latest libs in mavenCentral, and versionLib, which is the latest published snapshot (published to https://oss.sonatype.org/content/repositories/snapshots/). It's rare that changes in lib and the plugin are coupled (I can't think of any cases).

• use Maven Wrapper to not require pre-installed Maven when building

I trust your research on the tradeoffs of creating maven metadata manually vs shelling out to maven. I would love to switch to mvnw to better control what version of Maven gets used, if it's not a big job.

• Formatter / other steps / etc.

Without being inside Formatter, some of the FormatterStep can misbehave. They rely on always getting \n line endings, and Formatter is the code that maintains that contract.

I'm 100% fine with v1 of the maven plugin supporting only a single formatter step - this was definitely the right place to start. Just to establish what we'd need to reach near-parity with the gradle plugin:

spotless {
  encoding 'UTF-8'
  lineEndings 'GIT_ATTRIBUTES'
  generic {
    encoding / lineEndings
    license
    replaceRegex
    endWithNewline
    indent
  }
  java {
    eclipse / importOrder / googleJavaFormat
  }
  groovy {
    greclipse
  }
  kotlin {
    ktlint
  }
  scala {
    scalaFmt
  }
}

The natural way for Spotless to work is:

• define a set of files
• for that set, define a list of steps
• the generic steps are always available, and the other steps depend on what kind of files they are

I've got no idea if a structure anything like that is possible in the @Mojo annotation world. That seems like the next big question. Something like:

  <configuration>
    <java>
      <licenseHeader>...</licenseHeader>
      <eclipseFormatFile>${basedir}/build/eclipse-format.xml</eclipseFormatFile>
    </java>
    <kotlin>
      <licenseHeader>...</licenseHeader>
      <ktlint>...</ktlint>
    </kotlin>
  </configuration>

If we can get v1 to support at least all of <java>, that seems like a big win relative to supporting only a single step, but I'm fine with starting small.

Hi @nedtwigg, thanks for the response!

Do maven plugins require that they have no dependencies?

I think it's fine for a maven plugin to have dependencies. Shading was added because plugin needs lib and lib-extra when building. I thought maven plugin will be released together with lib and lib-extra and that's why can't depend on any specific version, because such version is released together with the plugin itself. Gradle plugin contains this in the build script:

if (!project.versionGradle.endsWith('-SNAPSHOT') && project.versionLib.endsWith('-SNAPSHOT')) {
  // gradle = release, lib = snapshot, therefore gradle should depend on the last stable lib
  compile "com.diffplug.spotless:spotless-lib:${project.stableLib}"
  compile "com.diffplug.spotless:spotless-lib-extra:${project.stableLib}"
} else {
  compile project(':lib')
  compile project(':lib-extra')
}

I think else branch is equivalent to what is achieved by shading - plugin depends on lib and lib-extra from the same version of the project/source. if branch however is missing and does not require shading - plugin can just depend on lib and lib-extra of version project.stableLib. Is my understanding correct? Do you think it is possible to completely skip shading and depend on project.stableLib or project.versionLib version of lib and lib-extra in the pom?

If we can get v1 to support at least all of <java>, that seems like a big win relative to supporting only a single step, but I'm fine with starting small.

Can't fully assess the complexity of other build steps right now. Feels like it should be easy to add them when configuration and Formatter infra is in place.

Feels like I have 3 main things to work on next:

• improve build of the plugin, use mvnw, prettify dependencies in pom
• use Formatter
• figure out how to pass nested configuration from pom.xml to the plugin

Does these look fine to you?

Thanks!

Oh and one other small question. Do you prefer to treat this PR as a feature branch? I can keep adding commits to this branch and it can be merged when stable. Another option is to prettify it a bit then merge then create other small PRs for subsequent improvements.

Do you think it is possible to completely skip shading and depend on project.stableLib or project.versionLib version of lib and lib-extra in the pom?

Yup! The point of the "else" block that you point out is to ensure that a published gradle plugin never accidentally depends on a SNAPSHOT version of the lib. That happened once on accident, so we put that in to prevent a relapse.

Do you prefer to treat this PR as a feature branch? Another option is to prettify it a bit then merge then create other small PRs for subsequent improvements.

My preference is to treat this as a feature branch for now. If we hit a forking point (maybe we should do A or B) then I'd be happy to merge.

I took the liberty of Formatter-fying your code. Feel free to revert or force push if it causes merge conflicts for you. Of the three todo's you identified (mvnw, Formatter, and nested config), this is the only one that I can help much with :)

Thanks @nedtwigg.

Added another commit that improves config handling. It makes use of nested xml tags, like you suggested. Will next simplify build process to use project.stableLib.

Hi @nedtwigg,

Thanks for adding docs! Last commit makes impl very close but not entirely follow them. Couple questions:

1. Maven docs recommend naming plugins like <yourplugin>-maven-plugin. Do you think it's fine to keep spotless-maven-plugin name instead of spotless-plugin-maven?

2. I'm not sure how to control step ordering without additional indirection in the pom.xml. Right now config looks like:

   <configuration>
     <encoding>UTF-8</encoding>
     <lineEndings>UNIX</lineEndings>
   
     <java>
       <encoding>US-ASCII</encoding>
       <lineEndings>WINDOWS</lineEndings>
       <steps>
         <eclipse>
           <file>${basedir}/build/eclipse-format.xml</file>
           <version>4.7.1</version>
         </eclipse>
       </steps>
     </java>
   </configuration>

   notice an additional <steps> tag. It makes configuration for all steps be injected into mojo as a list, so ordering is defined. Do you think it's ok to have this <steps> tag?

3. Current conventional naming of the plugin and mojo allows shorter command line invocation
   mvn spotless:apply. Do you like it this way or think it should be same as in docs
   mvn com.diffplug.spotless:spotless-plugin-maven:spotless-apply?

Thanks in advance for your answers.

I'll proceed with adding more steps (as docs suggest), tests and implementation of the check goal.

I changed the docs and artifactId to be "spotless-maven-plugin". I lean towards leaving the project's folder in the Spotless git repo to be "plugin-maven" - a little confusing, but seems a worthwhile compromise. I also added the <steps> container and changed task names to "mvn spotless:check/apply".

I'd be happy to help adding steps, but I don't know how to run tests. I've got an idea for how we could reuse a lot of the plugin-gradle tests, I'll push a skeleton in the next hour or two.

I added a first cut at an integration test framework for the maven plugin. Feel free to revert and do something completely different if you prefer. <sheepish>I think maybe we need your publish to a local maven repo back</sheepish>. I'm not sure how to make MavenRunner work without it...

I think CI build is now fixed. Build and tests for maven plugin will now use custom local repository instead of .m2. It is specified via command line -Dmaven.repo.local=<DIR> system property. This solves dependency caching problems and removed need for <repositories> and <pluginRepositories> in POMs. I can only see one drawback right now - it will download all dependencies every time because custom repo is initially always an empty folder.

What do you think about this solution?

Huzzah!!! You fixed it!!! I think that solution is okay, but not ideal. I added a slight tweak - we now cache the plugin-maven/build/localMavenRepository folder, but we delete the entire com.diffplug.spotless group before before saving the cache, so none of our artifacts ever gets cached.

Nice! I'll continue with include/exclude configs next. Current logic of scanning everything from project.basedir is most likely not ideal, especially for multi-module maven projects.

Hurrah! Extremely well-done. I'm ready to publish 0.x to mavenCentral. Get a round of feedback from users, get some example configurations to link to, and then we can publish 1.x and do some marketing. Any objections?

Could you please give me a day to play with the plugin and try it on couple projects? :)
I'm mostly interested in trying it out on multi-module maven projects.

Absolutely! Take as much time as you need.

I'm a bit uncertain about what to do with multi-module project support. It would definitely be nice for mvn spotless:check and mvn spotless:apply to work when executed in the root module and in sub-modules. Recommended way to achieve this is to create a separate module that only contains configuration (eclipse config, license header text, etc. in case of spotless) and depend on this module in the plugin declaration. Then all configs will be in the classpath and accessible to the plugin. Maven checkstyle and PMD recommend this way.

However, there exists a different, hackish way :) It is used by the Checkstyle plugin but is undocumented: source code. Here plugin iterates through the module hierarchy, which kind of violates the maven philosophy that module should only care about itself. This functionality makes it very convenient to specify files only once in the root module. Relevant checkstyle plugin issue and PMD issue contain a bit more details. Note that PMD authors closed the issue with "Won't fix" because this approach is considered to be a hack, though a very convenient one.

Despite it's convenience, later solution is a hack. I thought about implementing it but it's probably not a good idea, especially for the first iteration. Maybe it never needs to be implemented because it's not the "true maven way".

What do you think about this? I guess it's fine to proceed with 0.x release if we decide to not implement this thing.

It seems that even the authors of the Checkstyle plugin aren't very happy with their hack.

// MCHECKSTYLE-131 ( olamy ) I don't like this hack.
// (dkulp) Me either.   It really pollutes the location stuff
// by allowing searches of stuff outside the current module.

I'm an incredibly novice maven user, but the "official" multi-module solution described in your linked checkstyle and PMD docs doesn't seem terrible. A little verbose, but it seems like everything in maven is pretty verbose ;-) Ready to release when you are!

Yeah, I definitely agree that separate module just for configs is seriously verbose, even for Maven :)
I'm done with small fixes and ready for the release! 🎉

It should be available now on mavencentral as 1.0.0.BETA1. Unfortunately, there was already a 1.0.0.ALPHA which had been published to jcenter way over a year ago when we scaffolded this project out. Wanna do a sanity check, then we'll announce that #102 has been implemented and is ready for users!