METRON-746: Build Custom Checkstyle and IDE formatting settings

[justinleet]

Contributor Comments

Pretty much just editing the POM to actually set up Checkstyle with the Google Code Style, per the dev thread.

See Google Java Style Guide

Note that the Checkstyle version is set to 7.7 8.0. I've tested it with the plugin version and had no issues at all, but if anyone is concerned, we can lower it to either the default or another version.

IntelliJ warnings

To pull into IntelliJ, please see https://github.com/jshiell/checkstyle-idea#configuration
Essentially the instructions are to:

• Install the plugin (Available in IntelliJ's directly)
• Go into Settings -> Other Settings -> Checkstyle
• Change "Checkstyle version" to 7.7 8.0.
• Apply. Otherwise the new file won't match the version and an error will be thrown.
• Add a new Checkstyle file
  • This can either be a local file or a remote one (remote will attempt to update periodically, so if you don't want to repeatedly connect to the URL, download the file somewhere and load it in). Either works. For 7.7, use google_checks.xml Use the checkstyle.xml file included in the root directory of the project. This file is identical to google_checks.xml, but we may choose to change it in the future.
  • If you have errors, you most likely need to make sure you're set to the right version and have applied it.
• Select the checkbox for the new style
• Apply

To ensure it's setup properly, open a file, e.g. metron-platform/metron-common/src/main/java/org/apache/metron/common/dsl/functions/StringFunctions.java

New warnings should show up that are based on Checkstyle, e.g.

Checkstyle: WhitespaceAround: 'if' is not followed by whitespace. Empty blocks may only be represented as {} when not part of a multi-block statement

Code formatting

There are two options for code formatting. One is to import the checkstyle.xml file from the project root. The other is to import Google's IntelliJ editor settings for coverage. In practice, Google's direct IntelliJ settings seem to handle autoformatting better. The instructions are essentially the same for each. Note that if we choose to modify the checkstyle.xml we may also want to update the IntelliJ settings.

Google's setup is described at https://github.com/HPI-Information-Systems/Metanome/wiki/Installing-the-google-styleguide-settings-in-intellij-and-eclipse. The direct Checkstyle can be imported similarly, but choosing the CheckStyle Configuration when doing Import Scheme.

Required Follow-up

This will require updating the wiki at https://cwiki.apache.org/confluence/display/METRON/Development+Guidelines (Section 2.2). It needs to be switched to the Google code conventions, and the instructions above for formatting and warnings should be pulled in.

Testing

To get the checkstyle report do:

mvn clean install -DskipTests
mvn site site:stage-deploy site:deploy -Dmaven.javadoc.skip=true 

Navigate to file:///tmp/metron/site/checkstyle-aggregate.html in a browser.
You should see
The following document contains the results of Checkstyle 8.0 with google_checks.xml ruleset. and a set of warnings. In practice, most of them are fixed by batch autoformatting, per discussions, which will be a followup task.

Misc

No enforcement of Checkstyle is made, and in fact, by default checkstyle.xml sets everything to warning. The thought is that we'd need to pretty significantly reduce the number of warnings, and probably think through how we want to handle it.

Pull Request Checklist

Thank you for submitting a contribution to Apache Metron.
Please refer to our Development Guidelines for the complete guide to follow for contributions.
Please refer also to our Build Verification Guidelines for complete smoke testing guides.

In order to streamline the review of the contribution we ask you follow these guidelines and ask you to double check the following:

For all changes:

• Is there a JIRA ticket associated with this PR? If not one needs to be created at Metron Jira.
• Does your PR title start with METRON-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
• Has your PR been rebased against the latest commit within the target branch (typically master)?

For code changes:

• Have you included steps to reproduce the behavior or problem that is being changed or addressed?

• Have you included steps or a guide to how the change may be verified and tested manually?

• Have you ensured that the full suite of tests and checks have been executed in the root incubating-metron folder via:

  mvn -q clean integration-test install && build_utils/verify_licenses.sh 
  

• Have you verified the basic functionality of the build by building and running locally with Vagrant full-dev environment or the equivalent?

For documentation related changes:

• Have you ensured that format looks appropriate for the output in which it is rendered by building and verifying the site-book? If not then run the following commands and the verify changes via site-book/target/site/index.html:

  cd site-book
  mvn site
  

Note:

Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.
It is also recommended that travis-ci is set up for your personal repository such that your branches are built there before submitting a pull request.

[justinleet]

This appears to also work in Eclipse (similar deal: Install Checkstyle from Eclipse Marketplace, import GoogleStyle with the same instruction link as IntelliJ).

Having said that, Eclipse really dislikes some plugins in our POMs, and it seems like an Eclipse thing. I haven't dug in further, and to be blunt, am not interested in doing so unless someone really, really wants to work with Eclipse.

[justinleet]

Actually, that's a lie, autoformat works, but checkstyle doesn't highlight anything. Well, actually, Eclipse won't highlight any errors, including outright compilation errors. If someone has a working Eclipse environment, I can help setup checkstyle, but I'm not spending time debugging why Eclipse doesn't know how to import a cleanly cloned Maven project.

[justinleet]

Resolved conflicts.

@mmiklavc I think you said on another PR that you'd seen some issues with the Google formatting? Any update on that?

[mmiklavc]

@justinleet nothing in particular - what I noticed was that auto-formatting would format me right into breaking the checkstyle checks. I'd like to figure out how to get the two working consistently. I would like to spend zero time, or even negative time if it were possible, on thinking about code formatting as I add new classes or update existing code. ⌘-alt-L all day long, imho.

[justinleet]

@mmiklavc I didn't see any of that at all. All I saw was it either fixing things, or in certain cases (e.g. commas starting newlines) that it just left things alone. Autoformat won't fix everything automatically, because it won't touch certain things like the comma stuff, unfortunately.

If there's any examples, or a particular file, I can take a look at it and see if I can find a disconnect. I'll also take a look and see if I can find any examples where a checkstyle error from autoformat is produced (at least where there wasn't already one).

[ottobackwards]

Justin, should we have a local copy of the check style xml in the git repo itself? Then we could make changes... and they will be automatically setup?

[justinleet]

Yeah, we can. I just didn't bother for first pass. It's just a matter of grabbing the file, dropping it in, and referencing it. It's like two lines in the files.

I'll go ahead and pull in the base google checks for the version I pulled in.

[ottobackwards]

+1. This is a big step forward in getting all of this cleaned up. I would like to see it land early in the post 0.4.0 timeline so we can work through any issues.

[justinleet]

I removed the Apache header from the checkstyle.xml, because after research Checkstyle is LGPL and I don't even want to be in the questionable space of adding Apache headers to their config files (even though I've seen other projects do this). Including this is fine since it's just in our build tools, not part of our releases.

To reflect this, an exception for checkstyle.xml on the RAT check has been added.

Let me know if this is the incorrect strategy for handling this, and I'll update appropriately.

[justinleet]

@mmiklavc I still haven't found (significant) issues with autoformat + checkstyle, assuming both the checkstyle is setup and the autoformatting is setup (preferably with Google's IntelliJ settings). Are you okay with merging this in, and filing follow-on tickets for any issues? Or is there something in particular you'd like to see done here?

[justinleet]

@mmiklavc Any response to the above? I'm also happy to take a look through what you set up and see if there's anything missing in the instructions and so on, if that would help.

[mmiklavc]

Hey @justinleet, apologies for this slipping between the cracks. I'll run this again and see if there's anything specific I can find.

[justinleet]

As a note, I moved the checkstyle version up to 8 and will update the instructions.

@mmiklavc This'll be relevant if you look through anything. I don't know that it'll break on 7.7, because there weren't major changes to the file.

[ottobackwards]

Can I +1 this a second time?

[mmiklavc]

+1. I get why we didn't include the code formatting rules in the source tree like the checkstyle.xml (covers more than 1 IDE). But we should link to the setup instructions in our dev guide somewhere.