Spotless formatting implementation proposal

[johannescoetzee]

Background

The lack of automatic formatting in JavaParser makes it quite cumbersome to make any changes that involves code generation. This is because the code generators rewrite the core source files without preserving formatting, resulting in a very large diff with mostly unrelated changes. Since the code style in JP is inconsistent, it's currently impossible to fix this by formatting, so the only solution is to go through the diff and manually add files which contain relevant changes. This is both slow and error-prone (please see #4375 and #4387 for more discussion about this).

In #4202, there was the suggestion to use Spotless for automatic code formatting and I've played around with this for a bit and think I found a way to do this while minimizing impact on currently-open PRs (although there aren't that many in any case and most that exist are fairly small).

Solution

Reformatting JavaParser

Step 1: Add spotless with ratcheting to pom.xml

For this step, the idea is to add the spotless maven plugin configuration to pom.xml with the ratcheting option enabled, but to not run the plugin yet (the ratcheting option here is an option that takes a branch as input (in this case origin/master) and only applies spotless to files that have edits from the target branch). Merge this to master.

Step 2: Fully format JavaParser

Remove the ratcheting locally, re-run the code generators and reformat the entire project. Add spotless:check to the github workflow (possibly removing checkstyle, but there may be regions where they don't overlap). Merge this to master.

Technically removing the ratcheting here isn't necessary since all the regenerated sources would still be reformatted as well, but if we're going to do a huge reformat, then I think it's worth cleaning up everything. The commit from this should also still have ratcheting enabled since that's much faster than reformatting all files every time.

Step 3: Ignore the reformat in git blame

It's possible to ignore commits with git blame, so ignore the big format commit to avoid git-blame always pointing at the person who merged that instead of the person who actually wrote the relevant code. Github supports a .git-blame-ignore-revs file by default and this can also be set locally with git config blame.ignoreRevFile (which works for intellij as it uses cli git, but I havent' been able to find anything conclusive about eclipse support). See https://docs.github.com/en/repositories/working-with-files/using-files/viewing-a-file#ignore-commits-in-the-blame-view for more details.

At this point, the reformatting is done!

Updating open PRs or WIP changes

There may be a better way to do this, but this way worked well during a local test. I tested this with a test formatting branch and the old record-patterns branch, so will give a detailed walkthrough of that here. There are 2 important commits:

• -- the commit with spotless added to pom.xml, but without any formatting
• -- the commit which contains the full reformat

Step 1: Squash-rebase changes onto

This step makes dealing with conflicts later much easier. This can easily be done with git cli using:

git rebase -i <spotless>

This shows

pick c04439b27 Intellij refactor: PatternExpr -> TypePatternExpr
pick 42405a2a0 Refactor: PatternExpr -> TypePatternExpr in java.jj
pick bbc8f26bf Add TypePatternExpr codegen
pick 4b5e2fd1b Remove now-unused PatternExprMetaModel and Expression.XPatternExpression methods
pick ca9f594e9 Add PatternExpr abstract class and add it to the MetaModelGenerator
pick 8ef52f9b0 Add PatternExpr codegen
pick 07dd530ee Make TypePatternExpr a subclass of PatternExpr with codegen
pick 066700185 Update grammar to reflect new pattern inheritance structure
pick 7611bf5c1 Move TypePatternExpr type to PatternExpr
pick d3fb64086 Expect PatternExpr instead of TypePatternExpr in instanceof
pick b59fc9224 Replace TypePatternExpr with PatternExpr in javaparsermodel.contexts
pick c33390a5c Replace TypePatternExpr cast and instanceof where possible
pick c2f3eeaa3 Fix copyright feedback
pick 2a364d878 Remove unused imports
pick 3f33cef4c Add documentation for PatternExpr
pick bf90de2d6 Add tests for generated pattern methods

by default, but I want to squash everything, so I change that to

pick c04439b27 Intellij refactor: PatternExpr -> TypePatternExpr
s 42405a2a0 Refactor: PatternExpr -> TypePatternExpr in java.jj
s bbc8f26bf Add TypePatternExpr codegen
s 4b5e2fd1b Remove now-unused PatternExprMetaModel and Expression.XPatternExpression methods
s ca9f594e9 Add PatternExpr abstract class and add it to the MetaModelGenerator
s 8ef52f9b0 Add PatternExpr codegen
s 07dd530ee Make TypePatternExpr a subclass of PatternExpr with codegen
s 066700185 Update grammar to reflect new pattern inheritance structure
s 7611bf5c1 Move TypePatternExpr type to PatternExpr
s d3fb64086 Expect PatternExpr instead of TypePatternExpr in instanceof
s b59fc9224 Replace TypePatternExpr with PatternExpr in javaparsermodel.contexts
s c33390a5c Replace TypePatternExpr cast and instanceof where possible
s c2f3eeaa3 Fix copyright feedback
s 2a364d878 Remove unused imports
s 3f33cef4c Add documentation for PatternExpr
s bf90de2d6 Add tests for generated pattern methods

before exiting the editor.

Step 2: Amend squashed commit with formatting changes

./mvnw spotless:apply
git add .
git commit --amend 

Step 3: Rebase onto , keeping any changes

git rebase -X ours <spotless-formatted>

Done

From this point on, changes to master can be merged back into the branch as usual

[johannescoetzee]

If we go for this approach, then I'll add a comment to open PRs pointing them at this issue for the guide on how to update PRs

[johannescoetzee]

I've opened #4409 as a PR for step 1 of the reformatting process. It's fine if there end up being some commits merged between this one (assuming it's accepted, of course) and the big format. In this case, just replace the <spotless> commit in the process above with the last commit before the big reformat.

[johannescoetzee]

An idea I had from #4414 is that, if automatic reformatting is supported, we could probably add a CI workflow to run the code generators, reformat the code, and fail if the diff is non-empty. This would ensure that everything was generated correctly and that nothing is merged to master that would be overwritten by code generators.