xmlgraphics-general mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Chris Bowditch <bowditch_ch...@hotmail.com>
Subject Re: Checkstyle, Reloaded
Date Mon, 06 Feb 2012 09:14:34 GMT
On 03/02/2012 21:20, Glenn Adams wrote:

Hi Glen,

> which version of checkstyle are you using? there are two errors in parsing
> the proposed checkstyle file with 5.1;

Vincent says checkstyle v5.5 was used in his original e-mail.
>     <!--<property name="ignoreEnhancedForColon" value="false"/>  -->
>     <!--<module name="OneStatementPerLine"/>  -->
> once i fixed the checkstyle file to work with 5.1, i see that 4935 and
> 31935 new warnings/errors are introduced in trunk and in my i18n branches,
> respectively; clearly, this is going to require a large amount of editing
> to allow use of the proposed rules;
I agree that seems way too many new warnings/errors.

> many of the new errors I notice (in both trunk and my i18n branches) have
> to do with whitespace before or after '(', ')', and cast operations; i do
> not agree with enforcing the presence or absence of whitespace around these
> constructs; i happen to always use whitespace before and after parens,
> e.g., the following should produce no checkstyle warning:
> public int foo ( int a, int b, int c ) {
>    return bar ( a, b, c );
> };
> i would like whitespace after '{' and before '}' in an array
> initialization, e.g., both of the following should be permitted:
> int[] a = new int[] { 1, 2, 3 };
> int[] a = new int[] {1, 2, 3};
> i would like SimplifyBooleanReturn to be removed;
> i would like whitespace after BNOT produce a warning, e.g. both ! foo and
> !foo should be accepted without warning;
> i would like whitespace after DOT operator to be permissible, e.g., both
> x.y and x . y should be permitted;
> i would like empty blocks to be permissible, e.g., the following should be
> permitted:
> if ( test ) {
>    /* TBD - handle test is true */
> } else {
>    /* TBD - handle test is false */
> }
> i would like the arbitrary line length rule to be removed; i do not agree
> to 110 line length; or if you insist, i could accept 150;
> i do not agree with including MultipleVariableDeclarations rule; i
> routinely define multiple local variables in one statement, e.g., int x, y;
> i do not agree with requiring LocalFinalVariableName to match
> '^[A-Z][A-Z0-9]*(_[A-Z0-9]+)*$';
> instead, it should continue to match the currently used
> pattern "^[a-z][a-zA-Z0-9]*$";
> why are there two NoWhitespaceAfter rules?
>      <module name="NoWhitespaceAfter">
>        <property name="tokens" value="ARRAY_INIT"/>
>      </module>
>     <module name="NoWhitespaceAfter">
>        <property name="allowLineBreaks" value="false"/>
>        <property name="tokens"
>      </module>
> if you fix the above problems, then i will re-run on trunk and my i18n
> branch to check if there are any other issues that need to be resolved;
> On Fri, Feb 3, 2012 at 10:45 AM, Vincent Hennebert<vhennebert@gmail.com>wrote:
>> Hi All,
>> it is well-known that people are not happy with the Checkstyle file we
>> have in FOP. And there’s no point enforcing the application of
>> Checkstyle rules if we don’t agree with them in the first place.
>> I’ve finally taken on me to create a new Checkstyle file that follows
>> modern development practices. I’ve been testing it on my own projects
>> for a few months now and I’m happy with it, so I’d like to share it with
>> the community. The idea is that once we’ve reached consensus on the
>> Checkstyle rules we want to apply, we could set up a no warning policy
>> and enforce it by running Checkstyle in CI.
>> I’m also taking this as an opportunity to propose that we adopt a common
>> Checkstyle policy to all the sub-projects of XML Graphics. So once we’ve
>> agreed on a set of rules we would apply them to FOP and XGC immediately,
>> and eventually also to Batik, and keep them in sync.
>> We would also apply the rules to the test files as well as the main
>> code. Tests are as important as the actual code and there is no reason
>> why they shouldn’t be checked.
>> It is likely that the current code will not be compliant with the new
>> rules. However, most of them are really just about the syntax, so
>> I believe it should be fairly straightforward to make the code at least
>> 90% compliant just by applying Eclipse’s command-line code formatter.
>> Please find the Checkstyle file attached. It is based on Checkstyle 5.5
>> and basically follows Sun’s recommendations for Java styling with a few
>> adaptations. What’s noteworthy is the following:
>> • Removed checks for Javadoc. What we want is quality Javadoc, and that
>>   is not something that Checkstyle can check. Having Javadoc checks is
>>   counter-productive as it forces us to put {@inheritDoc} everywhere, or
>>   to create truly useless doc like the following:
>>   /**
>>    * Returns the thing.
>>    * @return the thing
>>    */
>>   public Thing getThing() {
>>       return thing;
>>   }
>>   This is just clutter really. I think it should be left to peer review
>>   to check whether a Javadoc comment is properly written, or whether the
>>   lack thereof is justified. There’s an excellent blog entry from
>>   Torsten Curdt about this:
>>   http://vafer.org/blog/20050323095453/
>> • Removed check for file and method lengths. I don’t think it makes
>>   sense to define a maximum size for files and methods. Sometimes
>>   a 10-line method is way too big, sometimes it makes sense to have it
>>   reach 20 lines. Same for files: it’s ok to reach 1000 lines if the
>>   class contains several inner classes. If it doesn’t, then it’s
>>   probably too big. I don’t think there is any definite figure we can
>>   agree on and blindly follow, so I think sizes should be left to peer
>>   review.
>> • However, I left the check for maximum line length because unreasonably
>>   long lines make the code hard to follow. I increased it to 110
>>   though to follow the evolution of monitor sizes. But as Peter
>>   suggested me, we probably want to keep it low in order to make
>>   side-by-side comparison easy.
>> • I added a check for the order of imports; this is to reduce noise in
>>   diffs when committing. I think most of us have configured their IDE to
>>   automatically organise imports when saving changes to a file. This is
>>   a great feature because it allows to keep the list of imports
>>   up-to-date. But in order to avoid constant back and forth changes when
>>   different committers change the same file, I think it makes sense that
>>   we all have the same configuration. I modeled this list after
>>   Jeremias’ one, that I progressively inferred from his commits.
>> Please let me know what you think. I’m inclined to follow lazy consensus
>> on this, and apply the proposed changes if nobody has objected within
>> 2 weeks. If anybody feels that a formal vote is necessary, feel free to
>> say so.
>> Thanks,
>> Vincent
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: general-unsubscribe@xmlgraphics.apache.org
>> For additional commands, e-mail: general-help@xmlgraphics.apache.org

To unsubscribe, e-mail: general-unsubscribe@xmlgraphics.apache.org
For additional commands, e-mail: general-help@xmlgraphics.apache.org

View raw message