Ok, reviving a thread that has been dormant for too long. Attached is an updated version of the proposed Checkstyle configuration. I removed/relaxed the following rules: • EmptyBlock (allow comments) • ExplicitInitialization (not automatically fixable) • NoWhitespaceAfter with ARRAY_INIT token • ParenPad
Note that I’m not happy with removing that last rule. I agree with Alexios that a consistent style makes reading and debugging easier. That wouldn’t be too bad if the original style were preserved in every source file, but this will clearly not happen. In fact, the mixing of styles has already started after the complex scripts patch was applied. I still removed the rule though. However, I left the MethodParamPad rule in order to remain compliant with Sun’s recommendations: http://www.oracle.com/technetwork/java/javase/documentation/codeconventions-141388.html#475 I’d also like to keep the NoWhitespaceAfter rule, as whitespace after unary operators increases too much the risk of misreading the statement IMO. Finally, I left the LineLength rule to 110. Long lines impede code readability too much IMO. They also make side-by-side comparison harder. I note that some even recommend to leave the check to 100. I think 110 should be an acceptable compromise. Please let me know what you think. Thanks, Vincent On 03/02/12 17:45, Vincent Hennebert 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
<?xml version="1.0" encoding="UTF-8"?> <!DOCTYPE module PUBLIC "-//Puppy Crawl//DTD Check Configuration 1.3//EN" "http://www.puppycrawl.com/dtds/configuration_1_3.dtd";> <!-- This configuration file was written by the eclipse-cs plugin configuration editor --> <!-- Checkstyle-Configuration: XML Graphics Conventions V2 Description: Slightly adapted from the Sun conventions. --> <module name="Checker"> <property name="severity" value="warning"/> <module name="TreeWalker"> <module name="ConstantName"/> <module name="LocalFinalVariableName"/> <module name="LocalVariableName"/> <module name="MemberName"/> <module name="MethodName"/> <module name="PackageName"/> <module name="ParameterName"/> <module name="StaticVariableName"/> <module name="TypeName"/> <module name="AvoidStarImport"/> <module name="IllegalImport"/> <module name="RedundantImport"/> <module name="UnusedImports"/> <module name="LineLength"> <property name="max" value="110"/> </module> <module name="GenericWhitespace"/> <module name="MethodParamPad"/> <module name="NoWhitespaceAfter"> <property name="allowLineBreaks" value="false"/> <property name="tokens" value="BNOT,DEC,DOT,INC,LNOT,UNARY_MINUS,UNARY_PLUS"/> </module> <module name="NoWhitespaceBefore"/> <module name="OperatorWrap"/> <module name="TypecastParenPad"> <property name="tokens" value="RPAREN,TYPECAST"/> </module> <module name="WhitespaceAfter"/> <module name="WhitespaceAround"> <property name="tokens" value="ASSIGN,BAND_ASSIGN,BOR,BOR_ASSIGN,BSR,BSR_ASSIGN,BXOR,BXOR_ASSIGN,COLON,DIV_ASSIGN,EQUAL,GE,GT,LAND,LCURLY,LE,LITERAL_ASSERT,LITERAL_CATCH,LITERAL_DO,LITERAL_ELSE,LITERAL_FINALLY,LITERAL_FOR,LITERAL_IF,LITERAL_RETURN,LITERAL_SYNCHRONIZED,LITERAL_TRY,LITERAL_WHILE,LOR,LT,MINUS,MINUS_ASSIGN,MOD,MOD_ASSIGN,NOT_EQUAL,PLUS,PLUS_ASSIGN,QUESTION,RCURLY,SL,SLIST,SL_ASSIGN,SR,SR_ASSIGN,STAR_ASSIGN,LITERAL_ASSERT,TYPE_EXTENSION_AND,WILDCARD_TYPE"/> <property name="ignoreEnhancedForColon" value="false"/> </module> <module name="ModifierOrder"/> <module name="RedundantModifier"/> <module name="AvoidNestedBlocks"/> <module name="LeftCurly"/> <module name="NeedBraces"/> <module name="RightCurly"/> <module name="EmptyStatement"/> <module name="MissingSwitchDefault"/> <module name="SimplifyBooleanExpression"/> <module name="SimplifyBooleanReturn"/> <module name="FinalClass"/> <module name="HideUtilityClassConstructor"/> <module name="ArrayTypeStyle"/> <module name="UpperEll"/> <module name="AnnotationUseStyle"/> <module name="ImportOrder"> <property name="groups" value="java,javax,org,org.apache,org.apache.batik,org.apache.xmlgraphics,org.apache.fop,com"/> <property name="separated" value="true"/> </module> <module name="DefaultComesLast"/> <module name="MultipleVariableDeclarations"/> <module name="OneStatementPerLine"/> <module name="EmptyBlock"> <property name="option" value="text"/> </module> </module> <module name="NewlineAtEndOfFile"/> <module name="FileTabCharacter"> <property name="severity" value="error"/> <property name="eachLine" value="true"/> </module> <module name="RegexpSingleline"> <property name="format" value="\s+$"/> <property name="message" value="Line has trailing spaces."/> </module> <module name="RegexpHeader"> <property name="severity" value="error"/> <property name="header" value="\/\*\n \* Licensed to the Apache Software Foundation \(ASF\) under one or more\n \* contributor license agreements. See the NOTICE file distributed with\n \* this work for additional information regarding copyright ownership.\n \* The ASF licenses this file to You under the Apache License, Version 2.0\n \* \(the "License"\); you may not use this file except in compliance with\n \* the License. You may obtain a copy of the License at\n \*\n \* http://www.apache.org/licenses/LICENSE-2.0\n \*\n \* Unless required by applicable law or agreed to in writing, software\n \* distributed under the License is distributed on an "AS IS" BASIS,\n \* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.\n \* See the License for the specific language governing permissions and\n \* limitations under the License.\n \*\/\n\n"/> </module> </module>
--------------------------------------------------------------------- To unsubscribe, e-mail: general-unsubscr...@xmlgraphics.apache.org For additional commands, e-mail: general-h...@xmlgraphics.apache.org
