On 12 Sep 2014 at 18:38:42, [email protected] ([email protected](mailto:[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 - Commons violations: http://sonar.xwiki.org/drilldown/issues/org.xwiki.commons:xwiki-commons - Rendering violations: http://sonar.xwiki.org/drilldown/issues/org.xwiki.rendering:xwiki-rendering - Platform violations: http://sonar.xwiki.org/drilldown/issues/org.xwiki.platform:xwiki-platform Thanks -Vincent > - 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 > > _______________________________________________ > > devs mailing list > > [email protected] > > http://lists.xwiki.org/mailman/listinfo/devs _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
