[hibernate-dev] Checkstyle and ORM

[Steve Ebersole]

Sanne, can you give me the low-down on this line?

<property name="cacheFile" value="${checkstyle.cache.file}" />

On Sat, May 16, 2015 at 1:26 PM, Steve Ebersole <steve at hibernate.org> wrote:

> Actually a few others I will add:
>
> 3) missing @Override
> 4) missing @Deprecated/@deprecated
> 5) missing braces
> 6) star imports
> 7) redundant imports
> 8) lower case 'l' for long literals (upper case is much more readable)
> 9) missing newline at end of file
>
>
> On Sat, May 16, 2015 at 1:18 PM, Steve Ebersole <steve at hibernate.org>
> wrote:
>
>> 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
>>>
>>
>>
>