[hibernate-dev] Checkstyle and ORM

[Steve Ebersole]

Yes, we use FindBugs too; I will start a discussion about it after this.  I
wanted to do CheckStyle first because its easier to work with IMO.  And yes
we use FindBugs because there are thins it can do that CheckStyle just
cannot (like checking for the use of non-localized
String#toUpperCase/toLowerCase that we consider a bug).  Certainly the 2
work in tandem.

Using filters is an interesting discussion as we do have this in ORM as
well.  The thing we keep running into is generated sources, but I can
certainly see what you do for tests as well.  BTW, your filter impl
mentions checking for "[not required for tests]", but I do not see that at
all in the CheckStyle config you linked[1]?

Yes, we should simplify the header and start enforcing it.  I think a first
pass for CheckStyle actually failing a build would be:
1) Use of spaces rather than tabs
2) No file header

Again, I said *fail* the build.  So these are the ones I would consider an
error initially.  We'd still have others come up as a warning.  Also in
Gradle these are not run depending on what task you run.  If you simply run
test, "code quality" (checkstyle, findbugs, etc) are not run.  CI runs the
check task as well.

Anyone want to "bang the table" to add more to the failure list?

[1]
https://github.com/hibernate/hibernate-search/blob/master/build-config/src/main/resources/checkstyle.xml#L172

On Sat, May 16, 2015 at 9:11 AM, Sanne Grinovero <sanne at hibernate.org>
wrote:

> 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
> _______________________________________________
> hibernate-dev mailing list
> hibernate-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/hibernate-dev
>