add spotless

[jetersen]

Implement spotless with sane defaults. I don't think we need to go down the rabbit hole of configuration. I added an example for <sort.nrOfIndentSpace>2</sort.nrOfIndentSpace>

I would like to remove it simply keep sane defaults and than if someone dislike it they can either override the configuration or use spotless.check.skip to skip.

At the moment this opt-out via <spotless.check.skip>true</spotless.check.skip>

If there is any desire we can also make it opt-in by setting it to default <spotless.check.skip>false/spotless.check.skip>

Not sure who should be pinged to properly review this.

@basil

• [x] Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
• [x] Ensure that the pull request title represents the desired changelog entry
• [x] Please describe what you did
• [x] Link to relevant issues in GitHub or Jira
• [x] Link to relevant pull requests, esp. upstream and downstream changes
• [x] Ensure you have provided tests - that demonstrates feature works or fixes the issue

[daniel-beck]

Implement spotless with sane defaults. I don't think we need to go down the rabbit hole of configuration. I added an example for <sort.nrOfIndentSpace>2</sort.nrOfIndentSpace>

I was so confused by this; to clarify: This is indentation of the pom, correct?

[jetersen]

I was so confused by this; to clarify: This is indentation of the pom, correct?

https://github.com/diffplug/spotless/blob/main/plugin-maven/README.md#sortpom Yes, that is indentation of the POM.

[basil]

This looks really great. Thanks for starting this effort. I will try this on a few of the plugins I maintain and see how it works. I want to explore the various settings we expose to users. In order to avoid disruption, I think any new functionality should be off by default, unless the vast majority of existing plugin POMs already comply. And as far as opting in to new functionality, I think there are two types of users:

• Those who care at least a little bit about the Git history, and would be happy to opt in to something like reordering dependencies or removing unused imports in Java files, but would not want anything more drastic.
• Those who don't care about the Git history at all and are OK with a "Git wall", including re-indenting pom.xml files and e.g. reformatting all Java files with Google Java Format.

Whatever interface we come up with should support both types of users, I think. For example, core has accepted import management and POM ordering from Spotless, but I doubt the core maintainers would be receptive to formatting every Java source file with Google Java Format. In contrast, Liam Newman did reformat one of his plugins with Google Java Format, and he was OK with the "diff wall". So we need to keep both types of users in mind in order for such a change to be adopted without complaints.

[timja]

Those who don't care about the Git history at all and are OK with a "Git wall", including re-indenting pom.xml files and e.g. reformatting all Java files with Google Java Format.

FWIW https://docs.github.com/en/repositories/working-with-files/using-files/viewing-a-file#ignore-commits-in-the-blame-view

git blame --ignore-revs-file .git-blame-ignore-revs

[basil]

I know, and I also use git blame -w by default. I am not one of the people who care about a "Git wall." But I know from experience that some others care strongly, and I expect there would be serious resistance to such a change from those people without a way of opting out.

[jetersen]

Regarding git blame, I suggest creating a PR afterwards to avoid commit sha changing on us in this PR if we use squash or rebase.

But definitely a good suggestion :)

[jetersen]

@basil my only concern if we open up the property list to fully configure each little aspect of spotless we will have 20-30 new properties.

My suggestion is to only have <spotless.check.skip>true</spotless.check.skip> to opt out and the rest is default. As I mentioned I would prefer to remove nrOfIndentSpace property. Defaults are there for a reason. No bickering, please 😅

[jetersen]

Apparently dependency order matters when it comes to commons-logging: see https://github.com/jenkinsci/plugin-pom/pull/132

@jtnord do you have any suggestion on your TODO, how should this be fixed so that order did not matter?

    <dependency>
      <!-- to avoid https://www.slf4j.org/codes.html#release warning -->
      <!-- TODO this would be better as a managed version of [0] -->
      <groupId>commons-logging</groupId>
      <artifactId>commons-logging</artifactId>
      <scope>provided</scope>
    </dependency>

[basil]

Yeah, I definitely don't think that the interface between the plugin parent POM and plugin POMs should expose every internal Spotless tunable that exists. That would be impossible to support in the long term as Spotless tunables evolve upstream. But I am not sure that an "all or nothing" approach of all Spotless checks or none of them is ideal either. For one thing, the existing "all" implementation in this PR doesn't satisfy me, as it doesn't run Google Java Format on every Java source file. :-) But to give another perspective, I think the percentage of people that are excited enough by this sort of thing to enable the "all" mode are relatively few, and I think the vast majority of people would stay in the "none" mode to avoid having to do any work. I think we might be able to curate a "middle ground" of less disruptive changes that might appeal to more people with a minimum amount of effort. If that "middle ground" is undisruptive enough, we could even consider enabling it by default.

[jetersen]

on every Java source file

I take it you mean test files? Yes, we should 😄

"middle ground"

I guess getting consensus for that would change over time, we should be prepared for changes to the settings than. Also hard to define initially. I don't feel too strongly about any specific settings, all I suggest we provide sane defaults. Without too much property overload.

[basil]

I'm saying that if I wanted to apply Spotless to my plugins, by "opting in" to some mode (whatever we decide to call it or name the property), I'd want it to run SortPom with maximum awesomeness (e.g., enforcing 2-space indentation, dependency sorting, all the good stuff) and Google Java Format with maximum awesomeness (e.g., full reformatting of every Java file in src/main and src/test with 2-space indentation and all other Google Java Format rules). I think the current PR stops short of that, which would hinder me from adopting it.

[basil]

The way I have been thinking about this in my mind is that we could define some "profiles" for formatting: e.g. "none" (no Spotless at all), "low" (some changes, like eliminating unused imports in Java code and conservative changes to pom.xml, but not going all out), and "high" (full-on dependency sorting and enforcing of 2-space indentation and Google Java Format rules). The default could be "low", as long as the changes are conservative enough to not bother too many people. People who are still bothered (there will always be a few) can opt out to "none", while source code formatting enthusiasts like myself can opt in to "high" to get very strict enforcement (at the cost of some extra effort).

[jetersen]

Profiles might be a good suggestion 🤔

[basil]

I think implementation wise it would be pretty easy, too: we could define a Jenkins-specific property to determine which profile to use, and then define several different Maven profiles with the different spotless settings, with the activation for each Maven profile being based on the value of the Jenkins-specific property.

[basil]

Oh right I keep forgetting that Maven profile activation works with system properties and not with Maven properties. But the gist of my idea is the same. You could instead enable or disable the Maven profiles for the different "Jenkins formatting profiles" in .mvn/maven.config.

[jtnord]

Apparently dependency order matters when it comes to commons-logging: see #132

Order matters period. it is not just about commons-logging but this is how maven and dependencies work.

if you have A depends on B1 and C depends on B2

if you declare your dependencies as A first then C you will get B1, if you decalare the dependenci on C first you get B1.

Then you have the enforcer upper-bounds errors which are a way to try and detect / prevent this.

Also you have the classpath order which is dependant (possibly) on the dependency order.

TL;DR enforcing a sort order of dependencies is incorrect.

@jtnord do you have any suggestion on your TODO, how should this be fixed so that order did not matter?

    <dependency>
      <!-- to avoid https://www.slf4j.org/codes.html#release warning -->
      <!-- TODO this would be better as a managed version of [0] -->
      <groupId>commons-logging</groupId>
      <artifactId>commons-logging</artifactId>
      <scope>provided</scope>
    </dependency>

could possibly be dependencyManage as a version number such as commons-logging-dependency-should-be-excluded-jenkins-provides-jcl-over-slf4j (or added to the banned dependency list in the enforcer plugin)?

[jtnord]

Oh right I keep forgetting that Maven profile activation works with system properties and not with Maven properties. But the gist of my idea is the same. You could instead enable or disable the Maven profiles for the different "Jenkins formatting profiles" in .mvn/maven.config.

IIRC this approach does not work for multi-module projects or aggregators, where you require different config per module.

[jtnord]

given this is a breaking change (it will cause builds to fail, and I guess many of them are not formatted as per the rules) I think it should be opt-in (then there is the question of how this impacts plugins that currently have it enabled. (with DB most people probably ignore any "upgrade notes" unless a build fails, and in the case we disable spotless unless you then set a flag, it could cause spotless to be silently disabled).

I am not against enabling spotless, but if different plugin authors all require different settings, is having the configuration in here the best solution?

[basil]

Closing in favor of #733.