Format Java source files automatically

[pugnascotia]

This PR introduces support for formatting Java code using google-java-format Eclipse JDT. Fomatting can be verified or executed out through Gradle. There is a CLI, and plugins for Eclipse and Jetbrains IDEs.

Formatting is executed with:

./gradlew spotlessApply

Checking is executed with:

./gradlew spotlessJavaCheck

Checking is also carried out during the precommit task.

I've updated CONTRIBUTING.md with some notes on formatting, IDE plugins, etc.

Sub-project needs to opt-in before the formatter will look at them - currently the list of opted-in projects is empty. This PR just adds the mechanism.

[elasticmachine]

Pinging @elastic/es-core-infra

[alpar-t]

This is great ! Thanks for working on it !

I added some comments, glad to help with any of them.

Does the tool re-format false == something to ! something ?
We feel strongly for the former and most tool would re-write that.
IDEA constantly complains about that with no apparent way to turn it off too.

[alpar-t]

PS: a few other examples of the what the new format changes would be useful too. Maybe add some files that have most changes ?

PS2: How long does it take for it to run ? Looking to see how much longer precommit would take. Can it maybe replace what checkstyle is doing ?

[pugnascotia]

The formatter is quick, I can reformat the entire codebase in about 4-5 minutes on my laptop. The IDE plugin formats a file instantaneously.

I've added somne choice reformatted examples. The formatting is actually quite conservative, and seems mostly concerned with whitespace, line length and ordering imports. From that perspective, it is nowhere near to replacing Checkstyle.

[alpar-t]

How long does it take to check that the format is the one we expect ?

[alpar-t]

Could we link directly to the plugins ?
Alternatively @mark-vieira do you know a way to generate an IDEA configuration that would at least suggests users to install the plugin ?

[mark-vieira]

I'm not aware of any. I suspect something like installing a plugin is not something most folks want to happen when importing a project. There are likely security concerns there.

[pugnascotia]

Spotless

I had another go at using spotless, and this time I got it to work. I don’t know what my problem was before, and I checked that Spotless does report errors arising from google-java-format (you get a great big stack trace, but the error is there). For plain old formatting violations, it prints a summary / report of the problems. I managed to bind it to precommit as well.

Note that I had to upgrade a jgit dependency, otherwise Spotless would call a non-existent jgit method when trying to print a diff and explode.

Groovy / Gradle

I also tried formatting the Groovy and Gradle files with spotless, which almost worked great, but it relies on an Eclipse formatter, and it did some very strange things with indentation, so I removed it again. One for the future perhaps.

Formatting notes

I also found a pathological formatting case in buildSrc/src/main/java/org/elasticsearch/gradle/precommit/TestingConventionsTasks.java. This file has some deeply nested expressions, which end up pretty unreadable under google-java-format with 4-space indents.The formatter adds a double indent when a statement wraps onto another line, which is mostly OK-ish, but in this file’s case, the code ends up smeared across the screen, so much so that it triggers the Checkstyle line-length warning. I committed the file to demonstrate.

Now, to address the immediate issue of Checkstyle, we could disable that check for Java files, because the formatter (usually) doesn’t allow long lines. That aside, if we proceed with formatting the codebase then we may want to follow up with some refactoring to make such files more palatable (e.g. extract expressions to vars). The other options is to switch to 2-space indents, which would probably reduce the size of diff significantly (if you ignore whitespace changes). What do people think?

On a similar note, the formatter will wrap comments to satisfy the line length constraint, which sometimes mangles the readability of comments somewhat. It’s not a big deal, and perhaps the team can fix up any particularly bad examples as they find them.

Issues with JavaDoc

Entertainingly, the formatter tries much harder to do something pretty with JavaDoc comments, and will even format HTML (and remove some closing HTML tags in the process, which I don’t agree with). Less entertainingly, the formatter will sometimes break a line after a <code> tag containing e.g. @something, which Java then misinterprets as a non-existent annotation and breaks the build. This can be worked around by editing the JavaDoc to cause the line to break elsewhere. There is an issue for this, and I left a comment:

google/google-java-format#84

[alpar-t]

This looks good to me.
We'll for sure get some oddly formatted files that we'll have to work on,
or maybe reformat as we discover them, but that's a fact of life that we can run into
Today, it's not a problem necessarily created by a format er ( even trough it could be aggravated )

The only problem I see is that making this happen all at once will be very difficult without generating lots of conflicts in all directions.
It would be nice if we had a way to do this on a project - by project or in some cases package by package basis to make it less disruptive.

E.x. we could specifically enable this for projects until we have them all and then enable for all by default ? Or enable for all and specifically disable for all existing project so new projects added will have it enabled by default and existing ones will migrate.

[pugnascotia]

@atorok I would have sworn I needed the afterEvaluate, but I took it out and it works 🤷‍♂

I've added a list of projects to format, and included only :build-tools. We could add projects to the list gradually and avoid dropping a giant conflict bomb on the team.

I also investigating the paddedCell() stuff, and wrote up some notes in the contributing guide. The issues were largely to do with comments and JavaDoc, and I fixed the issues that I found.

@rjernst @jasontedor Feels like this PR is in good shape, and the outstanding questions are:

• Does Core/Infra want to recommend this path to the wider team? (I can do this if we're happy)
• Do we want to keep 4-space indents? 😛

[alpar-t]

We will have to consider how this affects backporting.
If we run the tool both on master and 7.x will git be able to correctly track the changes and not make our life impossible when back-porting ?
I'm not saying it will, just saying we need to give it more thought.

[mark-vieira]

We will have to consider how this affects backporting.
If we run the tool both on master and 7.x will git be able to correctly track the changes and not make our life impossible when back-porting ?

This is a good point. The assumption would be that if we introduce this, we would first reformat all existing code. Then all subsequent PRs should be on top of that.

That said, we'd have to implement this back to all branches we intend to backport changes to, and also reformat those branches, or else as Alpar points out, backporting PRs from post-formatted branches is going to be a nightmare.

[jasontedor]

This is a good point. The assumption would be that if we introduce this, we would first reformat all existing code. Then all subsequent PRs should be on top of that.

That said, we'd have to implement this back to all branches we intend to backport changes to, and also reformat those branches, or else as Alpar points out, backporting PRs from post-formatted branches is going to be a nightmare.

It's for this reason that when we discussed this in the past we concluded the best time to make a transition like this is when we cut a major branch from master. If we did this here, we would hold this change until right before we cut 8.0 and 8.x from master. In this way, we would end up with 8.0, and 8.x, and master all formatted the same, and then backports from master to 8.x and 8.0, and 8.x to 8.0 would go through without pain since the branches can not have deviated between formatting master and cutting the branches. The only wrinkle is 7.last but I find that as time goes on (e.g., from master to 6.8) such backports become less and less clean anyway.

[pugnascotia]

So just so confirm, in that case we wouldn't gradually opt in Gradle projects over time, but instead format the whole repo just before cutting the branches. Correct? If so, I think I should send an email about this to the broader ES team sooner rather than later.

(I'm also looking for further feedback, if you have any, on the formatting itself, given the switch to the Eclipse JDT formatter.)

[pugnascotia]

I've trimmed this PR now so that it can be merged without doing anything 😁

[rjernst]

Could you please change the title now that we are no longer evaluating google-java-format? I left a couple minor comments.

[rjernst]

Shouldn't we have everything about the formatting doable by the IDE plugins?

[pugnascotia]

I don't think we'll have much need to enable this on a day-to-day basis, and it's outside the remit of the IDE plugins. This option lets Spotless cope when formatting isn't idempotent, which happened in a few places with GJF. It's a seperate thing from the configured formatting options.

[alpar-t]

@pugnascotia hope you don't mint that I back-ported this to 7.x on of my other back-ports wasn't clean because of this one

[pugnascotia]

I don't mind at all, we may very well end up formatting 7.x anyway.

[alpar-t]

Even if we don't, there's value in keeping the build code similar across branches to make it easy to pack-port improvements.