I object to this process. The question I raised is about 2 existing codestyle rules which we currently break the build over. I would like them removed and I made an argument why neither of them is defensible. In addition I believe we *should* break the build over a third rule which is not currently existing but I feel I have a justification for it.
I think you've hijacked the thread by replacing the simple question of whether these three policies are justifiable with a question of whether technology Y is a better fit for our needs than technology X. If you feel that changing build/validation technology is important, I invite you to begin a mail thread and I promise I won't hijack the discussion with talk about software licenses, programming languages or whathaveyou. Thanks, Caleb On 09/12/2014 06: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 >> _______________________________________________ >> devs mailing list >> [email protected] >> http://lists.xwiki.org/mailman/listinfo/devs > _______________________________________________ > devs mailing list > [email protected] > http://lists.xwiki.org/mailman/listinfo/devs > _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
