xmlgraphics-general mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Glenn Adams <gl...@skynav.com>
Subject Re: Checkstyle, Reloaded
Date Wed, 25 Apr 2012 16:03:00 GMT
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 <vhennebert@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-unsubscribe@xmlgraphics.apache.org
> For additional commands, e-mail: general-help@xmlgraphics.apache.org
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message