xmlgraphics-general mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Vincent Hennebert <vhenneb...@gmail.com>
Subject Re: Checkstyle, Reloaded
Date Mon, 06 Feb 2012 17:09:41 GMT
Hi Glenn,

Thanks for taking the time to look at this. Looks like we should be able
to reach a consensus without too much difficulty.

On 03/02/12 21:20, Glenn Adams wrote:
> which version of checkstyle are you using? there are two errors in parsing
> the proposed checkstyle file with 5.1;
>    <!-- <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;

Like I said most of them are purely about syntax and are easily solved
with a code formatting tool. Obviously I’m happy to run such a tool on
your own Git branches and submit a patch if that can help you.

> 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’d rather keep the rule, as it enforces standard Java style that will
be easily recognized by any Java developer. I also find the variation in
the use of whitespace to be one of the most distracting things when
reading code.

That said, if that really bothers you I would be ok with relaxing the
rule, except for the whitespace between a method call and the left
parenthesis, to make it clear that it’s a method call.

> 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};

Yep, no problem.

> i would like SimplifyBooleanReturn to be removed;

Hmmm. Ok.

> i would like whitespace after BNOT produce a warning, e.g. both ! foo and
> !foo should be accepted without warning;

I’d keep the rule. Allowing a standalone exclamation point is too
dangerous I think. Too easy to miss.

> i would like whitespace after DOT operator to be permissible, e.g., both
> x.y and x . y should be permitted;

Why? Note that it’s possible to break the line before the dot.

> 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 find that it’s just clutter, but I don’t really mind.

> 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’m afraid I’m not happy to go any higher than 110. Pascal actually made
a good point by saying that there is only a certain number of tokens
that a human being can grasp on one line.

Also, having to scroll horizontally is a real pain and greatly impedes
the readability of code. And if the editor is set up to automatically
wrap long lines then this ruins the indentation, which is not any

> i do not agree with including MultipleVariableDeclarations rule; i
> routinely define multiple local variables in one statement, e.g., int x, y;

I’d really rather keep the rule. Variables should be used (i.e.,
initialized) as soon as they are declared anyway. Otherwise there is
a risk that with time, the variable declaration appears further and
further away from where it is used.

> 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]*$";

Unless I missed something this is the default in Checkstyle 5.5.

> 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>

Because one rule allows a line break after the token, the other not. But
anyway, following your comment we would remove the former one.

> 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;

Let me know what you think. Hopefully others will also speak up to share
their views.


> 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