Vincent Hennebert wrote:

Hi All,

First of all let me start by saying many thanks to Glenn for the hours and hours of effort that he must of put into going through the code and resolving the checkstyle warnings. Thanks Glenn!

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.

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.

I personally don't have problems with the checkstyle rules and whilst I know there have been some unhappiness expressed in the past with the current rules set no one has put forward any improvements. Also I think it may be impossible to find a perfect set of checkstyle rules. In some cases the tool just isn't flexible enough in other cases it's impossible to reach consenus, so I tend to agree with Glenn here. Let's apply the parts of the patch we agree with now instead of waiting for a new set of checkstyle rules which may never be developed.


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.

I agree with Vincent here. I don't like the code being cluttered with CSOFF comments and it does defeat the purpose of checkstyle. Why not just accept that there will still be a few warnings left after the patch is applied. The patch is still a huge improvement on the current situation and we can try to address the remaining few warnings over time.


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.


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.
• 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.
• 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.

I am not happy to remove that file, although I must admit that my reasons are selfish. I have an older IDE that can't integrate with checkstyle 5 and I'm not ready to pay for a new license just for that reason. Is the checkstyle-4.0.xml file causing any problem? I don't see why it must be deleted.


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 think rushing things will just go against the honourable goal of
improving the quality of the software.

Thanks,

Chris


Vincent


Jeremias Maerki wrote:
I kind of agree with Glenn that we have a de-facto agreement on the
Checkstyle rules. We've adjusted them in the past and there's no reason
why we can't change it in the future. If Glenn's patch gets the issue
count down significantly so we can start to enforce the rules, then I'm
fine with it. But I don't doubt that the checkstyle file may profit from
some fine-tuning.

Right now, I have a couple of things that I need to commit and I
basically don't dare commit them as people may come screaming at me
in that case. So I want this resolved quickly. Vincent, are going to
process the patch? I have to do a few things first, but if you don't
have a chance to handle it by then, I'll take a look myself.

What I have a little problem with is Glenn's rant in the last paragraph.
Some Apache project don't even have Checkstyle rules. Furthermore,
having no Checkstyle issues doesn't equal high quality. High quality is
the result of an open process of developing software (The Apache Way).
There are no rules that we have to implement any particular technical
measures. But I'm not saying that Checkstyle doesn't help improve
quality. Then what is "high quality"? And we have to acknowledge that
over time, many people with different skills and coding styles have been
working on FOP and limited resources don't always allow the maximum
possible.

On 10.08.2010 13:13:08 Glenn Adams wrote:
Vincent,

I disagree with your proposed delay. First, there is an established
consensus on the rules, namely those that are in the existing
checkstyle-5.0.xml file. The reason they are the current consensus is that
they are there in the trunk, and nobody has objected to them. I do not care
to object to them at this time, and merely applied them as they stand.

I did nothing to change those rules in my patch, and saying that you wish to
effectively delay incorporating the patch until there is a consensus about
what is there already appears rather odd, if not counterproductive.

What is most important overall is to eliminate all warnings. Period. As fast
as possible. My patch does that, so please commit it without delay. We can
then, over time, decide if the existing rules are overly conservative or
overly liberal. But that is not going to be a useful way to spend our time,
it is much better to just use what is there, and when something goes outside
of that set, there are adequate mechanisms to deal with it, which I
described in my patch.

The alternative is to merely continue to propagate the current warnings.
Frankly, I was and am very surprised at the apparent lack of particularity
with respect to treatment of warnings. One of the six principles of "The
Apache Way" is "consistently high quality software". For me, every warning
is a black mark against quality. Let's not continue to propagate this state
of affairs. Now that FOP 1.0 has been released is the best time to move
forward, so why delay now?

Regards,
Glenn


On Tue, Aug 10, 2010 at 6:34 PM, <bugzi...@apache.org> wrote:

https://issues.apache.org/bugzilla/show_bug.cgi?id=49733

--- Comment #5 from Vincent Hennebert <vhenneb...@gmail.com> 2010-08-10
06:34:29 EDT ---
Hi Glenn,

Thanks for your patch. However, as I said we need to agree on a
project-wide
Checkstyle configuration first. Before enforcing a no-warning policy it is
necessary to reach consensus among all the developers on a set of rules
that
everyone is happy to follow.

We'll have a look at your patch once this is done. Meanwhile, I'll look at
the
parts that fix compilation warnings.

Thanks,
Vincent

--
Configure bugmail:
https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You reported the bug.




Jeremias Maerki




Reply via email to