[hibernate-dev] Checkstyle and ORM

[Sanne Grinovero]

One nice thing of checkstyle is that it's easily extended, for example
in Search we added a couple of checks, like to ban double whitespaces:
 - https://github.com/hibernate/hibernate-search/blob/master/build-config/src/main/java/org/hibernate/checkstyle/checks/regexp/DoubleSpacesCheck.java

Or to ban specific import statements:
 - https://github.com/hibernate/hibernate-search/blob/master/build-config/src/main/java/org/hibernate/checkstyle/checks/regexp/IllegalImport.java

For example this is useful to iteratively move away from
commons-annotations, like we ban its AssertionFailure to favor usage
of a different AssertionFailure within Search:
 - https://github.com/hibernate/hibernate-search/blob/master/build-config/src/main/resources/checkstyle.xml#L172

But checkstyle isn't enough, I for one would love to ban auto-boxing
from our main code (it's ok in tests) as I'd rather the syntax call
explicit attention to it.
Sometimes it's dangerous for performance reasons, or because of other
subtle API compability reasons like the JBoss Logger upgrade.. but
checktyle can't do such things, you need to add additional tools to
the mix, like FindBugs.

Have a look at the above linked checkstyle.xml for more ideas, but
sometimes I feel we've been too strict as it does fire back on quick
prototyping: it's very annoying to have your build fail for a couple
of style errors when you're deep in debugging some functionality..
I'd rather stick to it though, as it can be disabled locally with
appropriate flags and there finally are no more delays in pull request
reviews because of whitespacing.

To make it slightly less annoying I've made a CheckStyle filter which
allows us to enforce some selected rules only on non-test code:
 - https://github.com/hibernate/hibernate-search/blob/master/build-config/src/main/java/org/hibernate/checkstyle/filters/ExcludeTestPackages.java

We could similarly extend it to enforce a package-info.java for all
non-test, non .impl packages?

One which I would strongly enforce is about having the right copyright
headers. For example I've seen various hibernate-infinispan classes
using a non-Hibernate template. The traditional header includes the
year, which was sometimes problematic (like you'd copy an header in a
new class and mark it with a 6 years old copyright); so now we're
using a simplified copyright template which is shorter and has no
mention years:
 - https://github.com/hibernate/hibernate-search/blob/master/engine/src/main/java/org/hibernate/search/analyzer/Discriminator.java#L1-L6
(according to legal that's good enough)

-- Sanne


On 16 May 2015 at 00:19, Steve Ebersole <steve at hibernate.org> wrote:
> FYI : https://hibernate.atlassian.net/browse/HHH-9803
>
> Its not super high on my priority list, but I want to get to a point where
> we can start to fail the build on "serious checkstyle regressions".  Part
> of that is being more realistic with what is considered serious, and part
> of that is fixing up code.
>
> For example, we have quite a few warnings about spaces rather than tabs for
> indentation.  That's a serious one to me.  There are quite a few "unused
> import" warnings, again to me that's serious (it just looks sloppy).
>
> A few I have already disabled.  Checking that there is a package-info.java
> file for example.  I'd love to make a requirement that all API and SPI
> packages have one.  But as far as I know that detailing is not available in
> checkstyle, and I really dont overly care about package-info.java files for
> internal packages.
>
> These are just a few examples.  #909[1] (still running atm) is the first
> build with my preliminary work here.  Let me know if there are any checks
> anyone feels strongly about in one bucket or another.
>
>
> [1] http://ci.hibernate.org/job/hibernate-orm-master-h2/909/
> _______________________________________________
> hibernate-dev mailing list
> hibernate-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/hibernate-dev