I am in favor of clean code.  You can live with code that has warnings
as long as it compiles, runs, and doesn't crash, but it can be annoying
to other developers and should surely be removed by the time you're
calling it 1.0.
 
Important warnings like calls to deprecated methods and dead code should
be cleaned up.  Code should not give insificant warnings like an
arbitrary maximum line length.  Surely it's bad practice to make lines
unnecessarily long if they can reasonably be broken up, but it is often
easier and possibly more efficient for someone to code a line longer
than you would prefer.
 
Fleas on the couch should make the couch unusable.  Those would be your
errors.  Warnings are the stains.  You can sit on a stained couch with
no harm to yourself, though others would see your couch as less than
desirable, and if everyone allows new stains it would make your couch so
ugly that only the most desperate would bother to use it and they would
likely cringe in doing so.  I was a bit puzzled and annoyed when I first
tried to get the source and test an update, wondering why anyone would
publish code with so many reported problems, though I chose to ignore
the problem as others apparently had done since it seems to be the best
program available to do what I wanted to do and it worked despite the
problems.

________________________________

From: Glenn Adams [mailto:gl...@skynav.com] 
Sent: Wednesday, August 11, 2010 8:06 PM
To: fop-dev@xmlgraphics.apache.org
Subject: Re: [Bug 49733] [PATCH] resolve compilation, checkstyle,
javadoc warnings


Inline below.

On Wed, Aug 11, 2010 at 7:45 PM, Vincent Hennebert
<vhenneb...@gmail.com> wrote:


        Suppressing all the warnings at build time is a great goal that
I would
        love to see achieved eventually. This gives us an automatic way
to spot
        violations introduced in new code, which is better than the
informal
        check that developers do (or not...) before committing. But as I
said
        trying to achieve that goal now is premature.
        
        


once again, i disagree with your reasoning; i heard unanimous support
for this patch from other commenters, your reticence does not seem
warranted; Jeremias and Simon have both stated their support for taking
action to clean up the code base;

it is not premature to rid the codebase of warnings; in fact, one might
argue that it was premature to release FOP 1.0 with the existing
warnings;
 

        More or less everyone agrees that the current checkstyle file is
not
        satisfying. Jeremias says that he doesn't apply some rules
sometimes.
        I've done the same myself in a few occasions. So new warnings
are bound
        to appear shortly after this patch is applied.
        
        


to translate lack of satisfaction with the current checkstyles to mean
lack of acceptance is unwarranted; there have been no objections to it
as far as I can tell, so it is effectively accepted; I haven't heard you
or others proposing any concrete chantes to it, so it is accepted by
lazy consensus; moreover, you appear to believe (wrongly in my opinion),
that there could exist some future checkstyle rules set that was
uniformly satisfactory to all; that will never happen, and for you to
claim it should occur before taking action is nothing more than an
excuse to delay taking action;
 

        Once we agree on a new checkstyle file two things will happen:
Some
        rules may be removed and that may result into clutter CSOK
comments in
        the code; Are you happy to re-visit the code and remove them
afterwards?
        Some new rules may be put in place and that will result into a
whole
        bunch of new warnings, and we're back to square one.
        
        Globally disabling some Checkstyle rules by using CSOFF comments
is not
        an option to me. This kills the very purpose of a Checkstyle
file, which
        is to have a consistent coding style within the project and no
        distracting variations.
        


who said anything about using CSOFF to globally disable options? warning
suppression is a reasonable tool when used with appropriately, and
developers should be able to override rules as needed; the fact that the
comment remains in the code means it is easy to audit for these, and use
that information to evaluate divergence from norm and practice;
 

        We've been living with loads of Checkstyle warnings for years,
now what
        is this sudden urge to wipe off them all? If the goal is to
achieve and
        enforce zero warning, then I don't think this is doable in the
short
        term. If the goal is to improve the quality of the software,
then
        I don't see how putting unhelpful javadoc comments or even
disabling
        Checkstyle in some places will allow to achieve that.
        
        

 
You say it is not doable in the short term, but it would take you no
more than five minutes to apply and commit this patch. Instead of
offering excuses, why don't you actually do something about it to help
matters.

As for improving quality, if you walk into a house that is infested with
fleas, do you stop to wonder at the quality of the furniture? FOP is
infested with fleas. Let's exterminate them and move on to other
matters. Or would you rather sit with them and scratch all day?



        Anyway, from the quick look I've had at the patch, there are a
few
        things I don't agree with:
        * some methods marked deprecated were removed: this can't be
done
         arbitrarily and must follow some policy. Maybe this is fine in
the
         present case but that must be discussed first.
        


why? what policy was followed to deprecate them in the first place? why
were methods marked deprecated and then no alternative provided? why
were deprecated methods left in place that are no longer referenced? if
there are deprecated methods that are no longer referenced or the code
that references them is dead code, then they can and should be removed?
how is this different than removing old unused renderers? is there a
"policy" for removing old renderers? let's not invoke "lack of policy"
as an objection when you independently take action as a committer
yourself in cases that lack policy...
 

        * the @deprecated annotations that were on some other methods
were
         removed: there's a reason why they were there, which is to warn
         developers that those methods shouldn't be used in new code. If
the
         @deprecated annotations must be removed, then they should at
least be
         replaced with an appropriate warning in the Javadoc.
        


methods should not be deprecated unless there is a documented and
implemented alternative; this was not the case in certain of these
deprecations; if you feel that a specific deprecation should stay in
place, then argue its case here; don't just use this as a broad brush to
reject the entire patch;
 

        * before removing checkstyle-4.0.xml we must make sure that all
the
         developers are happy with upgrading their tools to Checkstyle
5. This
         will probably be the case but still, that should be done at
least
         through lazy consensus.
        


why? the existing checkstyle build rules had already been updated to
refer to CS 5 before this patch, so it is an unused and unneeded file;
why wait to remove dead files? 
 

        I will have a closer look at the patch and will apply the
        non-contentious parts. The rest will need to be discussed first.
        


I believe that is the wrong way to handle this patch. It should be
applied as a whole, then, once done, we can revisit specific issues, but
only if there is a viable reason to do so.
 

        I think rushing things will just go against the honourable goal
of
        improving the quality of the software.
        
        


They say justice delayed is justice denied. The FOP code demands
justice. And so do prospective developers. You seem to be laying
artificial hurdles into the path of contributors who are trying to make
valuable improvements. Why would you do that? What is the benefit of
delay?

Regards,
Glenn






Reply via email to