Bug 49268 - Use checkstyle to enforce common code style
Summary: Use checkstyle to enforce common code style
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 7
Classification: Unclassified
Component: Catalina (show other bugs)
Version: trunk
Hardware: PC Linux
: P2 enhancement (vote)
Target Milestone: ---
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-05-10 12:12 UTC by Marc Guillemot
Modified: 2010-07-22 18:32 UTC (History)
0 users



Attachments
Patch adding target checkstyle to main build.xml with a extrem minimal number of checks (10.74 KB, text/plain)
2010-05-10 12:12 UTC, Marc Guillemot
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Marc Guillemot 2010-05-10 12:12:31 UTC
Created attachment 25423 [details]
Patch adding target checkstyle to main build.xml with a extrem minimal number of checks

Tomcat code style is not consistent but it should be (it would give more time to discuss about unit tests rather than about code style).

Rather than to propose a huge patch in one time, I propose a simple one that:
- adds a checkstyle target to the build.xml
- adds a chestyle.xml configuration file (the "sun coding conventions") with nearly everything deactivated (otherwise it gives over 80000 style errors)
- activates only check for tabs

With this check the checkstyle target fails as a lot of files still contain a tab character. A find / replace by a committer makes far more sense than providing it in the patch as it would be more difficult to review.

Of course it would make sense to add this target to the default build.

Once this patch has been applied, work can start on other checkers, like the one verifying the presence of Version Javdoc tag in the right format.
Comment 1 Marc Guillemot 2010-05-21 09:30:31 UTC
What about applying this patch to have style conventions verified without discussion?
Commits here and there again with comments like:
"Tabs to spaces. No functional change."
just waste attention of all people (committers or not) following code changes. Additionally nothing guarantees that the work won't be lost, reverted by a future commit.
Comment 2 Konstantin Kolinko 2010-07-10 20:23:02 UTC
Thank you for the effort.
I won't comment on the project policies - that is to be discussed and decided on dev@.

Several comments on the patch itself:

1) Checkstyle version should not be hard-coded in build.xml.

> style="${checkstyle.home}/checkstyle-5.1/contrib/checkstyle-noframes.xsl"/> 

2) What is the origin of the res/checkstyle.xml file? There is no copyright or license notice there.

3) Contrary to the written "- activates only check for tabs", there are more checks that are enabled in the proposed checkstyle file.
Comment 3 Marc Guillemot 2010-07-12 01:18:01 UTC
(In reply to comment #2)
> ...
> 1) Checkstyle version should not be hard-coded in build.xml.

what is your preferred way? I wanted to avoid having an additional property but I can change it.

> > style="${checkstyle.home}/checkstyle-5.1/contrib/checkstyle-noframes.xsl"/> 
> 
> 2) What is the origin of the res/checkstyle.xml file? There is no copyright or
> license notice there.

this is the default checkstyle config file... with nearly everything as comments

> 3) Contrary to the written "- activates only check for tabs", there are more
> checks that are enabled in the proposed checkstyle file.

this is correct. The very few remaining checkers were the default one that weren't failing (what means that the style of some sources can be even worse than here ;-)).
Comment 4 Mark Thomas 2010-07-22 18:32:10 UTC
Fixed in trunk. Will be included in 7.0.1 onwards.