On Wed, Apr 25, 2012 at 11:46 AM, Vincent Hennebert <vhenneb...@gmail.com>wrote:
> On 25/04/12 17:03, Glenn Adams wrote: > > how does this differ from the current checkstyle-5.5.xml rules that are > the > > current default in fop? > > The following rules have been removed: > • prohibiting the usage of @author but we can add it back > i'm fine with keeping this in, since I already removed all existing usage of @author (in FOP files) > • CSOFF and CSOK > i do not accept removing these unless you are willing to remove all rules that trigger a warning/error in the absence of these pragmas > • Double (No idea what it is about. It doesn’t appear in the list of > available checks for Checkstyle 5.5.) > the full name is DoubleCheckedLocking, which is documented at http://checkstyle.sourceforge.net/config_coding.html#DoubleCheckedLocking > • FileContentsHolder (same) > this is needed for CSOFF/CSOK to work > • InnerAssignments > i don't mind removing this, particularly since I use inner assignments (with CSOFF/CSOK as needed) > • EqualsHashCode > i think this should stay, since it is part of the object contract, and exceptions (via CSOFF/CSOK) need to be explicitly documented > > The following rules have been modified: > • AvoidStarImport: severity changed from error to warning > ok > • ConstantName: removed log exception > could you elaborate? > • WhitespaceAfter: added typecast to follow Sun’s conventions > i don't accept this, particularly since it is widely used in FOP code (and I always use whitespace after typecast) i also don't accept changing LineLength back to 110; i believe someone proposed 130, which I can accept as long as i can disable entirely using CSOFF; i would prefer *no* limit > > > Vincent > > > > On Wed, Apr 25, 2012 at 9:44 AM, Vincent Hennebert <vhenneb...@gmail.com > >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: general-unsubscr...@xmlgraphics.apache.org > >> For additional commands, e-mail: general-h...@xmlgraphics.apache.org > >> > > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: general-unsubscr...@xmlgraphics.apache.org > For additional commands, e-mail: general-h...@xmlgraphics.apache.org > >
