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