Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable Spotless for automatic formatting #733

Merged
merged 4 commits into from
Apr 9, 2023
Merged

Conversation

basil
Copy link
Member

@basil basil commented Apr 1, 2023

Supersedes #538. This PR enables Spotless for automatic formatting, on an opt-in basis through the presence of a .mvn_exec_spotless file, with the following two formatters:

  • palantir-java-format (4-space indentation, 120 column line length)
  • Sortpom (2-space indentation, POM element ordering, dependency sorting, dependency management sorting, exclusion sorting)

I have tested this configuration in a dozen or so components and believe it to be the optimal path forward.

Anticipated Frequent Questions (AFQ)

Why do automatic formatting?

  • Because it reduces "nit" comments in code reviews, allowing contributors to focus on substance rather than style.
  • Because bot-authored code changes can be auto-formatted in a highly readable style.
  • Because it increases consistency across all repositories, so contributing across the Jenkins project feels familiar.
  • Because it eases the onboarding of new contributors.

Why not do automatic formatting?

  • If you do not like how the formatter laid out your code, you may need to introduce new functions/variables.
  • The formatter is not as clever as humans are, so it can sometimes produce less readable code.

Why make this opt-in?

  • Because we want people to be able to easily upgrade their plugin parent POM without having to cross an arbitrary formatting flag day (or opt out).
  • Because some people may not want to use automatic formatting, and we respect their choice.

How do I adopt this?

  • Run touch .mvn_exec_spotless && git add .mvn_exec_spotless.
  • Remove any existing spotless-maven-plugin configuration from your pom.xml file.
  • Run mvn spotless:apply.
  • Run git commit -a.
  • Test and merge the PR.
  • Create a .git-blame-ignore-revs file.

(This will be published in the release notes.)

Why not allow the user to configure any of this?

  • Because we want as much consistency as possible for those who do opt in to this. Those who want customization can simply ignore this feature and do formatting manually or with their own formatter configured as they wish.

Why make this opt-in via a file rather than a Maven property?

  • Because Maven profile activation works with system properties and not with Maven properties.
  • There is already precedent for this approach in our POM with the .mvn_exec_node and .mvn_exec_yarn profile activation files (which exist for the same reason).

Why use 4-space indentation for Java files rather than 2-space indentation?

  • Because most of the existing code in the Jenkins project assumes 4-space indentation and 120 column line length, so this results in a lower amount of disruption to the existing codebase.

Why use 2-space indentation for Maven POM files rather than 4-space indentation?

  • Because the Maven developers themselves use this convention.
  • Because XML tags get nested very deeply, and 4-space indentation would result in extremely wide lines that are difficult to read.

Why use Spotless?

  • Because it is the industry leading tool for this job.
  • Because we have good experience with it already throughout the Jenkins project.
  • Because it supports a wide variety of formatters.

Why use palantir-java-format instead of google-java-format?

  • Because we want 4-space indentation, and google-java-format's 4-space AOSP mode results in unreadable code for lambdas.
  • Because the Maven project is also using palantir-java-format successfully.
  • Because our experiments with google-java-format in AOSP mode compared to palantir-java-format in plugin-compat-tester showed that the latter was more readable.

Why use Sortpom?

  • Because the Maven developers themselves use Sortpom.
  • Because we have good experience with it already throughout the Jenkins project.

Why use Sortpom's POM element ordering feature?

  • Because the Maven developers themselves use it.
  • Because we have good experience with it already throughout the Jenkins project.

Why sort dependencies, dependency management entries, and exclusions?

  • On the one hand, this is somewhat risky and does change the classpath. On the other hand, it makes the code a lot more readable.
  • In practice, I have done this for many plugins and Jenkins core components without any issues.
  • I have had issues after sorting twice, once in PCT and once in ATH. In both cases the dependency tree was already fragile and contained multiple JARs each delivering the same classes. In each case debugging was painful but the fix was simple (excluding the duplicate) and the resulting POM less fragile than before the fix. I am willing to do this again if someone encounters a similar issue and needs help.

What if I want to use git blame after this?

Copy link
Contributor

@MarkEWaite MarkEWaite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great to me. Will allow me to remove the spotless configuration from plugins that I maintain.

Optional change for consideration on the spotless plugin version number.

pom.xml Outdated Show resolved Hide resolved
Co-authored-by: Mark Waite <mark.earl.waite@gmail.com>
@basil basil merged commit a76d8ec into jenkinsci:master Apr 9, 2023
MarkEWaite added a commit to jenkinsci/cloud-stats-plugin that referenced this pull request Apr 9, 2023
MarkEWaite added a commit to jenkinsci/elastic-axis-plugin that referenced this pull request Apr 9, 2023
MarkEWaite added a commit to jenkinsci/embeddable-build-status-plugin that referenced this pull request Apr 9, 2023
MarkEWaite added a commit to jenkinsci/implied-labels-plugin that referenced this pull request Apr 9, 2023
MarkEWaite added a commit to jenkinsci/markdown-formatter-plugin that referenced this pull request Apr 9, 2023
MarkEWaite added a commit to jenkinsci/nodelabelparameter-plugin that referenced this pull request Apr 9, 2023
MarkEWaite added a commit to jenkinsci/platformlabeler-plugin that referenced this pull request Apr 9, 2023
MarkEWaite added a commit to jenkinsci/pre-scm-buildstep-plugin that referenced this pull request Apr 9, 2023
MarkEWaite added a commit to jenkinsci/schedule-build-plugin that referenced this pull request Apr 9, 2023
MarkEWaite added a commit to jenkinsci/testng-plugin-plugin that referenced this pull request Apr 9, 2023
MarkEWaite added a commit to jenkinsci/versioncolumn-plugin that referenced this pull request Apr 9, 2023
MarkEWaite added a commit to MarkEWaite/authorize-project-plugin that referenced this pull request Apr 10, 2023
@basil basil deleted the spotless branch April 12, 2023 05:57
MarkEWaite added a commit to MarkEWaite/pipeline-steps-doc-generator that referenced this pull request Apr 25, 2023
MarkEWaite added a commit to MarkEWaite/pipeline-metadata-utils that referenced this pull request Apr 26, 2023
MarkEWaite added a commit to MarkEWaite/groovy-postbuild-plugin that referenced this pull request Dec 16, 2023
jenkinsci/plugin-pom#733 describes the benefits of
automated source code formatting and how automated source code formatting
is now provided by the Jenkins plugin POM.
MarkEWaite added a commit to jenkinsci/groovy-postbuild-plugin that referenced this pull request Dec 16, 2023
jenkinsci/plugin-pom#733 describes the benefits of
automated source code formatting and how automated source code formatting
is now provided by the Jenkins plugin POM.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants