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.
Chris
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"
value="BNOT,DEC,DOT,INC,LNOT,UNARY_MINUS,UNARY_PLUS"/>
</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<[email protected]>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: [email protected]
For additional commands, e-mail: [email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
