I've been working on and off on updating our usage of Checkstyle, since we haven't really updated our checkstyle configuration since we first started using it, other than minor tweaks. In the latest version of the Checkstyle tool several useful checks have been added, which I'd like to enable. On 09/12/2014 04:15 AM, Caleb James DeLisle wrote: +0 on this one, it's an extra safety net, but one that I agree is a bit too cumbersome, and I'm not a big fan of defining private constants just to work around this rule. +1. All public code should be thoroughly documented, and private code should only document what's not obvious. +1, but there's currently a bug in that rule: https://github.com/checkstyle/checkstyle/pull/274 More rules to enable in https://github.com/xwiki/xwiki-commons/pull/9 -- Sergiu Dumitriu http://purl.org/net/sergiu _______________________________________________ devs mailing list devs(a)xwiki.org http://lists.xwiki.org/mailman/listinfo/devs

Hi, I haven’t had time to read this thread yet. Very generally: - checkstyle is a dead tool - the sonar guys have been reimplementing the rules in a better way - sonar offers more rules (pmd, findbugs, etc) - I’ve started defining rules we may want to use in this email thread: http://xwiki.markmail.org/thread/vnggoomrbqbfaojn - I’ve already excluded rules, listed on http://design.xwiki.org/xwiki/bin/view/Proposal/DefineSonarRules - One next step is to look at http://sonar.xwiki.org to see the remaining violations and decide which one we want to remove - Another step will be to decide to use the sonar maven plugin to act as a replacement for the checkstyle maven plugin and fail the build in case of errors - The list of currently enabled rules are available here: http://sonar.xwiki.org/coding_rules#qprofile=java-xwiki-57409|activation=tr… Thanks -Vincent On 12 Sep 2014 at 18:27:50, Sergiu Dumitriu (sergiu@xwiki.org(mailto:sergiu@xwiki.org)) wrote: > I've been working on and off on updating our usage of Checkstyle, since > we haven't really updated our checkstyle configuration since we first > started using it, other than minor tweaks. In the latest version of the > Checkstyle tool several useful checks have been added, which I'd like to > enable. > > On 09/12/2014 04:15 AM, Caleb James DeLisle wrote: >> Hi all, >> >> This is partially an extension of the previous mail thread >> ``Checkstyle audit audit'' where in the discussion, I formed a few thoughts >> about checkstyle rules which should be added and removed. Since that thread >> is very cluttered now, I wanted to begin a new one specifically explaining >> the changes which I think we should make. Comments are requested. >> >> >> All but the most trivial Codestyle rules all create friction which slows down >> work. They are however valuable because they identify likely mistakes and make >> it easier for others to read one's code. Still we must periodically review our >> rules and identify both new rules which can help us and existing rules which >> are more bother than they are help, either in general or in specific to the >> experience level of our team. >> >> >> Changes I would like to make: >> >> #1: Remove "duplicate strings" checkstyle rule. This rule is supposed to trap >> people who are hardcoding constants throughout their code instead of defining >> them in one place. However when writing this email, I tried to find a coherent >> example which is much improved by hoisting the string value up into a constant >> and I cannot think of a single example. False positive examples spring to mind >> immediately, usually log/error messages or on strings which are sure never to >> change (at least not before the next major refactoring). "fixing" these errors >> makes the code worse because one can no-longer read top to bottom, one must >> scroll up and down between constants and code. I think this rule is more harm >> than good, especially for our team's experience level. > > +0 on this one, it's an extra safety net, but one that I agree is a bit > too cumbersome, and I'm not a big fan of defining private constants just > to work around this rule. > >> #2: Set class/method/field javadoc requirement to exempt private and package >> private entities. When I write code, I want to do everything the easiest way >> that could work. When I read code, I want every comment to be giving me >> important and up-to-date information. If checkstyle makes me write comments >> which teach programming, I'm going to be mad at checkstyle, if I try to debug >> your code and the important details/warnings are mixed in with a rash of CS-101 >> babble, I'm going to be mad at you. In either case it takes me longer to do >> what I set out to do. >> The solution of requiring javadoc only on public classes/interfaces also helps >> me when developing because I need to think more deeply about the interfaces >> which I am exposing to the world. "write javadoc here" or >> "this needs an @param tag" implies >> "hey, did you really intend to make that public API?". >> I think the change not only lifts the burden on the programmer but also >> actually helps checkstyle achieve it's goals more efficiently. > > +1. All public code should be thoroughly documented, and private code > should only document what's not obvious. > >> #3: Add rule requiring use of 'this.' when accessing class fields and remove >> rule preventing variable names which shadow fields. This change makes checkstyle >> *more* strict but I believe the cost is reasonable and the benefit is high. >> Reading your code I immediately know whether you're accessing a field or a >> variable which tells me whether there is reason why your function might behave >> differently depending on the state of the object. In light of this change, >> there is nolonger any reason to concern ourselves with shadowing of field >> names since we cannot access them this way so I think the new rule should >> be accompanied by the removal of the shadowing rule. > > +1, but there's currently a bug in that rule: > https://github.com/checkstyle/checkstyle/pull/274 > > More rules to enable in https://github.com/xwiki/xwiki-commons/pull/9

On 12 Sep 2014 at 18:46:33, Sergiu Dumitriu (sergiu@xwiki.org(mailto:sergiu@xwiki.org)) wrote: The decisions are separate: we’re already using Sonar and we’ve never really taken the time to define the rules we wish applied there on http://sonar.xwiki.org So there are several related but orthogonal decisions: 1) Define the sonar rules for the web site 2) Decide if we wish to automatically fail the build when a sonar rule fails. Basically this means deciding to use sonar as THE one tool for rule checking. An alternative would be to use the Maven Findbugs plugin, Maven PMD plugin, etc. But it would be a lot less powerful than Sonar. For example in Sonar you can combine the rules and say things like “fail the build if such rule and such rule fail or if the TPC is below that value or …”. Now since Sonar also contains the checkstyle rule in the end we will need to decide which checkstyle rules we wish to use too, so this work has to be done anyway. It’s just not the only one since there are more rules (and a lot more interesting than the checkstyle ones, in other tools).

On 09/12/2014 12:52 PM, vincent(a)massol.net wrote: Two question I have about Sonar: 1. Can it fail the build when I run mvn install locally? I don't want to have to wait hours to find out if I have all the spaces in order.

If it's only an online asynchronous tool, we'll get in the same situation as with our CI failures, we'll pay attention to failures for a few weeks, then most devs will give up and check the build status only when pinged or when it's a BuildFixingDay.

2. Does it run faster or slower than the Checkstyle plugin? If slower, by how much? The build already takes too much time for devs to have the patients to build more than one submodule before committing, we don't want devs to skip running checks locally completely.

A tool still being maintained and released isn't dead. 

The fact that there's healthy competition is good, not bad, for a project. And we're not bound to using the most complex tool available, we should use the one which works best for us, where "best" should be agreed on by the community. There should be a separate vote about switching from Checkstyle to Sonar. If you send such a vote, we can put this on hold until we decide one way or the other. It's kind of backwards to agree on the rules before agreeing on the tool first. On 09/12/2014 12:38 PM, vincent(a)massol.net wrote: -- Sergiu Dumitriu http://purl.org/net/sergiu _______________________________________________ devs mailing list devs(a)xwiki.org http://lists.xwiki.org/mailman/listinfo/devs