On 12 Sep 2014 at 18:46:33, Sergiu Dumitriu
([email protected](mailto:[email protected])) wrote:
> 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.
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).
Thanks
-Vincent
> 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
> _______________________________________________
> devs mailing list
> [email protected]
> http://lists.xwiki.org/mailman/listinfo/devs
_______________________________________________
devs mailing list
[email protected]
http://lists.xwiki.org/mailman/listinfo/devs
