Format code base

[jbduncan]

Five potential options:

1. Use a Maven plugin for google-java-format, like https://github.com/coveo/fmt-maven-plugin.
   • Advantage:
     • Formats code automatically.
     • Easiest automatic option to setup.
   • Disadvantage:
     • Doesn't order imports according to Google Java Style Guide or remove unused imports (Sort imports and remove unused imports spotify/fmt-maven-plugin#11).
2. Use Checkstyle's support for compliance with the Google Java Style Guide.
   • Advantage:
     • Has supported Google Java Style Guide the longest, so probably has most extensive implementation.
   • Disadvantage:
     • Cannot automatically reformat code AFAIK - one manually formats code until it complies with Checkstyle.
3. Use google-java-format from the CLI.
   • Advantage:
     • Formats code automatically.
     • Orders imports properly and removes unused imports.
   • Disadvantages:
     • Hard to setup - requires knowledge of Shell and Travis to manually integrate it with Travis.
     • No native checking capability - CLI tool is unable to verify at CI time that code meets the expected format, so one would need to use a downstream tool which fixes this like sormuras/google-java-format (Add dry-run mode and --set-exit-if-changed option google/google-java-format#106).
4. Format manually with google-java-format before each new commit.
   • Advantage:
     • Little to no setup required.
   • Disadvantage:
     • Easy to forget to format code - no help from CI.
5. Migrate to Gradle and use Spotless plugin.
   • Advantages:
     • Formats code automatically.
     • Orders imports properly and removes unused imports.
   • Disadvantages:
     • Hardest to setup - requires migrating from Maven to Gradle.

[jbduncan]

I've found a solution based on option (3) at google/conscrypt (https://github.com/google/conscrypt/blob/c240f160e4346d57d9ba3b0b3c08da687812922f/.travis.yml). I'm currently in the middle of writing up a PR for review.

[jrtom]

(Sorry for the delay.)

tl;dr: (1) and (5) sound OK, (3) sounds incomplete but perhaps you have more to say about this.

(1) sounds reasonable to me. JUNG isn't required to follow the Google Style Guide. (It also seems plausible that the author of that plugin might accept a PR to add the ability to follow the guide.) And most IDEs are pretty good at reminding people to clean up imports.

(2) doesn't sound very useful; I prefer having something that does the formatting to something that tells us how we're violating the expectations. :)

(3) might be OK but I'd prefer it if we can do the formatting pass before the PR stage, to make it easier for people to fix any egregious weirdness that the formatter might impose.

(4) is basically abandoning the idea of an automated solution.

(5) is potentially of interest if there are other significant reasons to use Gradle over Maven. I'm not a big fan of Maven but it does have the nice property that we already have it set up for JUNG, but I'm open to hearing reasons why we should migrate. (Although that should be considered as a separate issue.)

[jbduncan]

Yes, I realise I hadn't explained the full story for (3). Allow me to elaborate.

Since I wrote up this issue, I discovered that the team at google/conscrypt have apparently found a workaround for google-java-format's lack of verification ability. They seem to be running a Shell script at Travis CI time which runs google-java-format on the codebase and then runs git diff --exit-code to check for formatting changes.

If I understand it correctly, if the error code code is 0, then it means that there are no formatting changes, and thus logically all the code is properly formatted already. However if the error code is 1 (or maybe some other value), then it means that running google-java-format did produce formatting changes, which means google-java-format wasn't run beforehand on the codebase, which logically equates to a failed validation check.

I found this solution of theirs here.

If this sounds too complicated for you, then I'd be happy to pursue (1) or (5) instead.

[jbduncan]

With regards to (5), the two biggest reasons I personally use Gradle over Maven are as follows:

1. IMO it's easier to use, at least because of Gradle Wrapper, which eliminates the need to install Gradle beforehand.
2. Gradle build scripts can contain programmatic Groovy code, which means one doesn't necessarily need to write a plugin or connect to Ant (as one would need to with Maven) if one needs to break Gradle conventions a bit.

[jrtom]

Those do sound like nice properties of Gradle, but perhaps not enough to warrant converting the existing Maven setup to use them. (If Guava ever decides to convert over, I may piggyback on their effort.)

Regarding the original topic: the approach that (3) is taking seems reasonable. My reservation is that I would prefer that the formatting happen before the code shows up in a pull request, ideally automatically, which is why I favor solutions like (1) and (5). (I'm assuming, perhaps incorrectly, that (3) does not actually modify the pull request itself; does it?)

That said, at least having a check at PR time (perhaps using CheckStyle, presuming we can set that up with Travis) might also be useful. So maybe a hybrid of (1)+(2)?

[jbduncan]

Those do sound like nice properties of Gradle, but perhaps not enough to warrant converting the existing Maven setup to use them. (If Guava ever decides to convert over, I may piggyback on their effort.)

That's fair enough.

It seems more likely to me that Guava will switch to Bazel than Gradle in the future (google/guava#2850). So if anything, Bazel would probably be the way to go for JUNG.

Regarding the original topic: the approach that (3) is taking seems reasonable. My reservation is that I would prefer that the formatting happen before the code shows up in a pull request, ideally automatically, which is why I favor solutions like (1) and (5). (I'm assuming, perhaps incorrectly, that (3) does not actually modify the pull request itself; does it?)

No, AFAICT, (3) doesn't actually modify the PR itself - when it runs google-java-format on Travis and checks if any changes were made, it automatically discards any changes found at the end of the job.

So AFAICT, (3) would be similar to options (1) and (5).

With options (1) and (5), contributors would need to manually run mvn com.coveo:fmt-maven-plugin:format or gradlew spotlessApply to format their changes before submitting them as a PR - that's unavoidable, since it should be Travis's job to check that contributions are properly formatted, not to format them for the contributor.

Similarly, for (3), contributors would need to download & install google-java-format first, and then run git ls-files | grep -E '\.java$' | java -jar path/to/google-java-format-{version}-all-deps.jar --replace from the terminal (assuming Unix or Cygwin) before submitting the PR.

Although, having said this, I realise now that asking contributors, especially those running Windows, to go through the kerfuffle needed to format PRs via option (3) would be too restrictive, since it's rather involved and, for Windows, requires a specific setup. Being able to just run mvn com.coveo:fmt-maven-plugin:format or gradlew spotlessApply would be more accessible. So (3) no longer seems like a desirable option to me.

[jbduncan]

I'm also open to a hybrid like (1)+(2) if we discover that Checkstyle has a check or two that would be useful for us.

[jbduncan]

So (1)[+(2)] looks like the most sensible option to me, even though I personally dislike how it can't order imports and remove unused imports like (3) or (5) can.

[jbduncan]

I now have no objections at all to (1), because issue spotify/fmt-maven-plugin#11, which was looking into adding import sorting and unused import removal, has been resolved now.

[jbduncan]

Closing since this is resolved as of #114.