tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Konstantin Kolinko <knst.koli...@gmail.com>
Subject Re: [VOTE] Add Checkstyle ruleset and make code cleanups!
Date Sun, 11 Jul 2010 01:20:04 GMT
Adjusted the thread subject.
Was: Vote: Add Checkstyle roleset and make code cleanups!


2010/7/8 Peter Ro├čbach <pr@objektpark.de>:
> Hi,
>
> after the discussion about code style
> (http://tomcat.markmail.org/thread/2c7lkzmpcuxqpgjj), I think that we must
> vote for this fix:
>
> https://issues.apache.org/bugzilla/show_bug.cgi?id=49268
> It includes a starting point for a very limited checkstyle ruleset.
>
> +1: progressively introduce checkstyle with checks for which a consensus
> exists (first TabSpacePolicy, @Version format)
> 0 : More discussion needed
> -1: no checkstyle integration. Tomcat shoudn't have code style rules

1. What is TabSpacePolicy? It is not listed at:
http://checkstyle.sourceforge.net/availablechecks.html

There is FileTabCharacter though.
http://checkstyle.sourceforge.net/config_whitespace.html#FileTabCharacter


2. There is NO consensus on @Version format.

Last time when it was discussed, about a year ago,
there was slight agreement to get rid of those tags at all.
http://marc.info/?t=124692531300003&r=1&w=2

Though, when I actually was performing the change recently I went for
a more safe route of just replacing $Date$ with $Id$.

One caveat with $Id$ that I noted in the last several months:
when filename is long, "@version $Id$" can occupy more than 80
characters and is wrapped when reformatting the file in IDE,  thus
breaking this keyword.  In several such cases I replaced $Id$ with
$Revision$.


3. From message in "Re: r960104" thread:
http://markmail.org/message/rkznrp2cnfkd4eob:
> FileTabCharacter -> currently 146 failures

In what packages are those files?

Fixing them can be discussed and done first,
before enabling any checkstyle nags in the project.


4. Is it possible to exclude some packages from checkstyle checks?

E.g., org.apache.tomcat.util.bcel ?


5. Is there experience whether checkstyle checks run fast, or there
are noticeable delays?

The "Re: r960104" thread was about preventing commits that have wrong
whitespace.   It probably means that checkstyle is run automatically
by IDE, or by the buildbot. Do others have positive experience with
such configurations?


Based on the above
[x] 0 : More discussion needed

I am +1 if someone else wants to add a separate "checkstyle" target to
build.xml.

I do not mind against checks that already succeed for the existing
code. Though if they always succeed they are not really useful.

Regarding the checks that fail -- before enabling the check I would
like to discuss whether we can fix the code in TC7, and whether we can
backport the fix.  At least the affected files have to be listed.



Best regards,
Konstantin Kolinko

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org


Mime
View raw message