DISCUSSION :: Should we use maven-checkstyle to enforce Java coding style?

[nickcmaynard]

Description

Our code is mostly formatted according to Google Java Style, but we can enforce this pretty easily using maven-checkstyle: https://maven.apache.org/plugins-archives/maven-checkstyle-plugin-2.16/

Swagger-codegen version

All.

[nickcmaynard]

Am happy to make this happen, if people think it's a good plan.

[wing328]

Sounds good to me.

[nickcmaynard]

So I have an outline plan for this, and it looks something like this:

1. Put in place the POM changes that execute checkstyle, but only WARN on failures - this is PR Add Checkstyle to POM with Google settings advice #4047.
2. We say "PRs don't increase the number of warnings" - ie. any changes don't fail checkstyle.
3. I'll submit a once-off PR that:
4. Fixes all point-in-time checkstyle failures.
5. Changes the POM checkstyle behaviour to ERROR (and fail the build).

What do people think?

[ePaul]

Hmm, is there some way to automatically fix the violations? I would like to not just annoy people, but give them an easy way to also fix the problems then (like reformat the problematic files).

[nickcmaynard]

I do have an uncrustify config that could go a long way to achieving that. Will test it, use it, and submit it as part of stage 3.

[cbornet]

If we use google style, we can use google-java-format with aosp option for 4-spaces indent

[nickcmaynard]

That could well be better...

[nickcmaynard]

@wing328 Now that #4047 is merged, we can update the PR template to say "check your code for checkstyle failures". We can do that now or when (3) gets done (working on that), but I'd suggest we do it sooner rather than later.

[nickcmaynard]

Actually, after playing with google-java-style and uncrustify for an hour or so, I'm suspicious that we won't get to (3) against these strict Google styles - not without many man-hours. A number of these checkstyle failures aren't automatically fixable - they include things like variable naming, etc.

I'll take a(nother) look at this, but we might be better served by moderating Google's style - some reasonable compromises.

[wing328]

Yup, agreed with you that it would take a lot of man-hours to complete (3). We'll leverage the community to work on it bit by bit.

[nickcmaynard]

#4214 takes care of the vast majority of the warnings once the formatter has been run once. The remainder are (AFAICS) not "formatting" issues - they're things like import ordering, enforcement of block-style if statements, etc.

I propose that once we merge #4214, a maintainer immediately runs mvn formatter:format on the codebase and commits the results of this. Once that's done, we can hook the formatter into a standard mvn lifecycle phase - commented config already exists in swagger-codegen/pom.xml.