Introduce unified code formatting for all IDEs

[tobous]

Currently, the code formatting rules used in Eclipse do not match the code formatting rules used in IntelliJ. This leads to an unevenly formatted codebase. Furthermore, this can lead to edit-wars/unnecessary formattings when working on code of an IDE specific part of Saros with a different IDE.

As far as I can tell, all project except "intellij" are formatted according to the Eclipse formatting rules.

[stefaus]

I think we have two options for this.

1: Use this Plugin to import the existing Saros Eclipse Format Rules into IntelliJ.
I've tested it, but it has some problems with formating using the existing Saros config.
I think this occures because the Saros format rules File is to old and this would change with using 4.6+ Eclipse (and update the existing rules file).

But as I already expressed at the Stand-Ups I don't like the current formatting rules.

Therefore my preferred option 2: Use the google java format rules. This obsolete discussions about format details, brings a widely used ruleset and it comes with a stand alone formatter.
https://github.com/google/google-java-format

They provide Plugins to format code directly in Eclipse and IntelliJ.
It works from the CLI making it IDE independent and even provide gradle support, so we could check format violations for each check in.

[m273d15]

Option 2 looks good! The rule set is immutable and can be checked during the CI process 👍

[tobous]

That sounds like an interesting solution, but we should discuss the contained formatting when considering using predefined immutable settings.

For example, I am not really happy with their default indentation width being 2 spaces (instead of 4).

[stefaus]

Yes, the indentation was also unusual to me.
But the gain: Block indentation (+2 spaces) is easier to distinguish from continuation lines (+4 spaces).

Changing the format rules would need a project fork, making things difficult.

[srossbach]

Why do we need a fork ? We could modify the source to our needs, compile it and put the result in one of our Git Repositories. That would at least ensure that every uses the same version.

[tobous]

As I already said, I don't like that the code formatting enforced by "Google Java Style" is not configurable/adjustable.

An alternative would be Checkstyle (GitHub, Website). I have to read up on it more, but from the looks of it, it allows to set up code style profiles that can be enforced at build time. (There is a pre-build profile that imitates the Google Java Style format that would be adjustable.). This can be done be integrated into our gradle build through a plugin, meaning it could be done automatically and would fail the build if certain criteria of the code style were not met.

The main drawback of Checkstyle is that, as the name already suggests, it just checks the code style. It is not a standalone formatter. For this we would still have to rely on IDEs. Both IntelliJ and Eclipse have an option to import Checkstyle configurations into the local code formatter (I only tried it with IntelliJ).
I don't think this can be done IDE independent. (The website mentions that there is a Vim plugin, but it has been abandoned since around 2003.)

This means Checkstyle is not a perfect solution either, but I think it is worth a consideration. (It is defensively a better solution than patching/forking the google java style plugin in my eyes.)

For me, not being able to format the code without an IDE is not a problem, but I don't know what your workflows are like. What are your opinions on using Checkstyle?

[srossbach]

I think without a default formatter there is to much room for edit wars (e.g one prefers short line, the next one does not even care etc.). And marking a build as failed because of some violation regarding the layout of the code is in my opinion a very bad idea.

[tobous]

What do you mean with "without a default formatter"? With a codestyle profile imported into the IDE formatters, we would have basically the same situation that we have currently (especially since you have to import the eclipse profile manually currently anyway, meaning basically nothing would change except you would also have to install the checkstyle plugin). The files defining the code style would still be shared through the git repository. The only difference to the current situation would be that we have a configuration that can be applied to both IDEs and that can be checked on the build server.

And regarding failing builds: If the codestyle settings are sensible, I don't see why we shouldn't fail builds that don't meet the defined code formatting, at least on the build server. This saves us the hassle of discussing basic formatting during reviews. If the guidelines are not met, the server gives a negative review for us. The only important part would be that these checks are only run after the normal build (as build errors should take precedent in my opinion) and that it is communicated clearly to the developer that the (travis) build failed because the formatting was wrong.

For local builds I understand your point that failures due to wrong formatting could be counterproductive. For checking the formatting locally, we could use a gradle task to call checkstyle or use the IDE plugins.

[srossbach]

So do you mean we should use Checkstyle to verify that the developers are using a specific code formatter (like the Google one) or do you suggest only to use Checkstyle to make a build fail if the code have some serious formatting issues ? In my opinion it is still frustrating if you build fails on Travis because of formatting violations when you do not have a formatter available. So it basically the workflow could look like this:

while CheckStyle is not happy on your local code layout : do some modifications

result = ?

It is like when you are enforcing JavaDoc

/** Make tool XYZ happy so the build will not fail **/

public T methodWithVeryComplexLogic(....)

[tobous]

I would suggest to use Checkstyle to

• Define a specific code formatting that is shared through the git repo
• Allow developers to check for this code formatting locally (through a gradle task or IDE plugins)
• Allow developers to format their code according to these defined formatting rules iff they are using an IDE that supports such a functionality (IntelliJ and Eclipse both do)
• Fail Travis builds if the defined code formatting is not met when pushing to the repository.

But we would have to test how well these code formatting rules are applied by the IDE formatters before enforcing them for Travis builds. I ran into an issue where the automatic formatting was in conflict with the defined rules. But this might have been a problem with my setup.

[m273d15]

I think without a default formatter there is to much room for edit wars (e.g one prefers short line, the next one does not even care etc.).

I totally agree. As long as the formatting is consistent for all IDEs (Eclipse, IntelliJ, Vim/Emacs) I am fine. Everybody, has its own taste and discussions about personal taste are tedious. I think the easiest solution is to create poll where we vote for "immutable google style" or "customized formatting style". The question whether the formatting should be checked during the CI process is independent of this question.

In my opinion it is still frustrating if you build fails on Travis because of formatting violations when you do not have a formatter available.

If we want to you use the corresponding gradle plugin everybody would be able to run the formatter. Furthermore, everybody could use the standalone formatter (either Google format or checkstyle) in a git hook. I think we already do this for JavaScript since #264 .

It is like when you are enforcing JavaDoc /** Make tool XYZ happy so the build will not fail **/

I don't see this point if we talk about formatting, because the formatting is adjusted automatically.

[tobous]

I think we need to be more careful in this discussion if we are talking about code formatters (that actively format the code) or format checkers (that only check whether the code is formatted in a certain way).

Google Java Style only provides code formatting (not format checking) that can be done by the developer locally. It does not provide any way of checking this style on the build server, meaning we would have to enforce these formatting rules as part of the build or git check (and I am torn on whether this is really a good idea).
We could theoretically use Checkstyle with the Google Java Style configuration, but I don't know whether this is a good idea as there are no guarantees that the defined checkstyle rules (the rule set was created in 2017; I don't know whether there were any significant changes since then) exactly match the google plugin.

Checkstyle mainly provides code format checking (not formatting) that can be done by the developer locally and by the build server. It does not provide an independent way of automatically applying these formatting rules, meaning we would have to use other tool to do the automatic formatting (mainly IDEs).

[m273d15]

Google Java Style also supports formatting and checking. See the cli definition:
use --set-exit-if-changed for checking and --replace for formatting.

[srossbach]

It is like when you are enforcing JavaDoc /** Make tool XYZ happy so the build will not fail **/

I don't see this point if we talk about formatting, because the formatting is adjusted automatically.

See Tobias answer. I was just talking about using CheckStyle without any automated formatter at all.

[Drakulix]

There is no much reason in having a defined code style, if it is not enforced using e.g. CI testing. I am sometimes guilty of this myself, as I do not always want to open eclipse (as it is not my preferred IDE/editor), when working on a cli application (the server) and therefor sometimes forget running the current formatter.

Being able to run a formatter as e.g. a git hook or part of the build process is a huge bonus for me, but I would personally also be fine with anything I can integrated in any major ide, like checkstyle.

However I am voting for the google formatter, because we get an actual formatter - not just a style checker - as a cli tool.

@tobous Do you have any examples, where we would potentially like to adjust google's formatting rules? If the goal is just to have a consistent style, because anything else is a matter of taste, then this seems irrelevant to me. Even if some of those rules might look ugly for the majority here, it cannot be much worse then the current situtation in my opinion.

[tobous]

As I already said, the only problem that I have with the google java style formatting is that default indentations are only 2 spaces.

This would not be a problem if we could use the google AOSP style (which is basically the google java style with 4 space indentations). While this option is supported by the google java style Gradle plugin and the IntelliJ IDEA plugin, it is not supported by the Eclipse plugin. There is an issue and a PR to fix this, but there were no updates on both in a couple of month, so I am not confident that it will be fixed any time soon.

[tobous]

On the other hand, AOSP style uses 8 spaces for line continuations, which looks very weird as well. It wastes a lot of space and makes lines longer than they have to be, which is especially cumbersome because the google java style uses a lot of line continuations.

To be honest, I think I just need to get used to the new formatting. The default google style is fine with me.

But, before using it, we should also check how nice the formatter plays with other languages (for example the javascrips files in our repo).

It still seems weird to me to automatically apply the formatting (it just seems weird to me that these changes happen out of sight), but that is probably something I will have to get used to as well (this is already the default behavior in Eclipse anyway). The question is where to apply this formatting. As part of the build is not really sufficient if the idea is to always automatically apply the formatting as developers could theoretically never execute the code before committing. Furthermore, we only want to apply this automatic formatting on the user side. I am strongly opposed to the idea of changing code/commits (by applying the formatter) on the build server.

@srossbach What are your opinions on using the google java style plugin (with the default google style or AOSP style)? To be more specific, what do you think about the idea of using the google-java-style plugin to format the code locally (either manually or automatically) and to check on the build server whether the check-in code adheres to the formatting rules (and fail the build otherwise in my opinion).

Another question is how we are going to add this new formatting to our repo. It would be a sensible idea to also re-format all java files with the new formatter. This would however cause problems for any open pull request as the resulting merge conflicts are probably quite complicated to resolve. This is especially bad for long-running commit chains, like the work of @pdler, which contain a lot of changes but are still far of from being merged.

[Drakulix]

The question is where to apply this formatting.

I agree that during the build is weird and I feel similar about changing code on the server.

Typically a good way to enforce formatting is using a git pre-commit hook (which I would prefer over a build hook).

This ensures, that:

• either formatting is checked, when trying to craft a commit, and the commit is rejected in case there is something wrong with the style, forcing the dev to run the formatter
• or the hook formats the whole codebase just before commiting the result.

Both, when used by all developers, would theoretically enforce every commit to be formatted correctly, but I strongly prefer the first solution, because otherwise if anyone is not using the git-hooks the commit might contain unrelated formatting changes.
You can always skip a git-hook by using an additional command line argument, but the first solution always makes you aware, that something is wrong.

Additionally hooks reside in Project/.git/hooks/ and are local to a project and not globally set on a system, which is a plus in my opinion. However I think there is no way to check in files in the .git folder. To manage our hooks using git, we would need to create a folder in the repository (lets say git-hooks), that everyone has to symlink manually to .git/hooks.

[m273d15]

But, before using it, we should also check how nice the formatter plays with other languages (for example the javascrips files in our repo).

I tried the cli, eclipse and intellij version of the google-java-format and all versions produced the same result and only changed the java files.

Is everybody fine with the following process?

• Using IDE specific plugin/CLI formatter during development
• Ensuring the formatting by a local git hook (using the cli version java -jar google-java-format-1.6-all-deps.jar --dry-run --set-exit-if-changed <files to format>) which is managed in the repository
  Checking only staged files contained in the commit
• Travis CI checks the formatting (as in the git hook) before the build and test stage. It would be also possible to check the formatting of each commit of a PR in case one commit contains invalid formatting.

[m273d15]

• Full re-formatting and introduction of the Travis check with [BUILD] Check java formatting in Travis CI #313
• New documentation with [DOC] Add information about Google Java Formatter #314
  See the development environment docu for information how to set up the new formatter within your IDE