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 18:03:36 GMT
On Wed, Apr 25, 2012 at 11:46 AM, Vincent Hennebert <vhennebert@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 <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
> >>
> >
>
> ---------------------------------------------------------------------
> 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