ACCUMULO-3771 Configure automatic java code style formatting

[mikewalch]

Pull request for ACCUMULO-3771

To make code review easier, you should add &w=0 to your URL to remove any white space differences. This should bring down the files changed to only 88 files.

[asfbot]

Accumulo-Pull-Requests #69 FAILURE
Looks like there's a problem with this pull request

[joshelser]

Looks like this reintroduces all of the lone-spaces in Javadoc that we've previously worked to remove...

[mikewalch]

The plugin is configured to use the existing contrib/Eclipse-Accumulo-Codestyle.xml. I will figure out a way to not introduce lone-spaces...

[joshelser]

IIRC, there was an issue with Eclipse itself not adhering to the checkstyle. Not knocking what you've done -- my hunch was that it might be the maven plugin itself not doing what it's told to do.

[ctubbsii]

There are two main problems with the Eclipse formatter:

1. It doesn't strictly adhere to the line length restrictions.
2. It adds a trailing space to indent empty javadoc lines.

We've already accounted for the line length in the checkstyle rules by making it much larger than the Eclipse formatter. With this patch, I think we could probably just ignore trailing whitespace (and drop the corresponding checkstyle enforcement), since the formatter will remove the ones we mainly care about anyway (the javadoc ones are annoying, but there's not a lot of good options so long as Eclipse fails to fix that issue). We could just strip whitespace with sed after the formatter executes.

One thing I'd like to see, though (for the purposes of a review) is just the pom changes, separate from any resulting formatting changes (I'm sure I can do this with some command-line Git Fu, though, so it's no big deal).

I did see that the formatting significantly changed some lines that looked deliberately formatted to make it more readable. For those lines, we may wish to add the @formatter:off / @formatter:on tags to retain existing formatting.

I'm also curious what happens with the -Pthrift profile activated. Will this plugin reformat the generated thrift code? Maybe that's okay, but it might be annoying. If the generated thrift code were in a separate package, it could be avoided more easily, though.

I'm overall +1 to the idea, but it might need some tweaking.

I especially like the idea that we could focus our style choices on readability, because writing to conform to a particular style doesn't matter any more (write in whatever style you like and the build will fix it).

[keith-turner]

One thing I'd like to see, though (for the purposes of a review) is just the pom change

The PR has two commits. Commit 4b69fba has just pom changes. Is that what you are looking for?

It adds a trailing space to indent empty javadoc lines.

Would you happen to know if this if fixed in later versions of Eclipse? This formatter plugin currently relies on Eclipse 3.8.1 code to do the formatting.

[ctubbsii]

Yes, that commit is exactly what I was looking for. :)
No, it's not fixed in later versions of Eclipse. However, later versions do fix the "Save Actions" to strip trailing whitespace a bit, so they no longer compete with the Formatter, but that doesn't help us.

[keith-turner]

I found Eclipse bug 49619, but I can't tell what was done from reading the issue comments.

We may have to resort to executing a sed command after the format step :)

[ctubbsii]

That Eclipse bug also doesn't fix the same issue in regular block comments (only javadocs).

[asfbot]

Accumulo-Pull-Requests #70 FAILURE
Looks like there's a problem with this pull request

[mikewalch]

I just pushed an updated PR with following changes:

• Modified the configuration of the maven-java-formatter-plugin in pom.xml to use a newer version (3.10) of the eclipse.jdt.core. This reduced but did not eliminate the number of trailing whitespace changes. The number of files changed by the formatter was reduced from the 550 to 170.
• I agree with Christoper about the formatter. It mostly removes whitespace. The only place where it adds whitespace is in block comments. To get checkstyle work, I modified the "Trailing whitespace" checkstyle rule ignore whitespace after an asterisk *.
• Excluded thrift and proto buffer files in the formatter plugin configuration

I reviewed all of the files that were changed after running the formatter. While they look good to me, feel free to comment on any formatting that should remain and I can add // @formatter:off comments to those sections of the code.

[keith-turner]

I modified the "Trailing whitespace" checkstyle rule ignore whitespace after an asterisk *.

Thats nice, instead of completely removing the check

[asfbot]

Accumulo-Pull-Requests #71 SUCCESS
This pull request looks good

[ctubbsii]

This might be more specific: ^(?!\s+([/][*])?[*])\s+$

[ctubbsii]

I issued a PR against Mike's branch which addresses the remaining trailing whitespace issues and helps make this work a little nicer.

My only remaining concern after that is merged is that if this is not also applied to 1.6 and 1.7 branches, things will merge cleanly into master which are not properly formatted. So, we might want to apply these changes to 1.6 and 1.7?

[mikewalch]

I tried to create a build-tools module. However, I ran into a problem where java-formatter-plugin tries to run on parent pom but fails as the build-tools jar has not been created. The parent pom needs to be configured so that build plugins (formatter, checkstyle, find-bugs, etc) are not run in the parent and only run in child modules after the build-tools jar is built. I will create a separate JIRA issue for this.

[asfbot]

Accumulo-Pull-Requests #72 FAILURE
Looks like there's a problem with this pull request