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.

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.

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

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