how does this differ from the current checkstyle-5.5.xml rules that are the current default in fop?
On Wed, Apr 25, 2012 at 9:44 AM, Vincent Hennebert <[email protected]>wrote: > 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 > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: [email protected] > For additional commands, e-mail: [email protected] >
