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, [email protected] wrote:
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=true|languages=java|s=createdAt|asc=false
Thanks -Vincent
On 12 Sep 2014 at 18:27:50, Sergiu Dumitriu ([email protected](mailto:[email protected])) 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
-- Sergiu Dumitriu http://purl.org/net/sergiu