Hi Glen,

I haven't looked at your patch, perhaps Vincent is aware of something that some 
of the other committers are not.  But it sounds like you have made a good 
contribution to the codebase, and certainly an improvement over the current 
state.  If committing the patch helps you move forward more swiftly with 
further contributions to the project then I am all in favour of some flea 
powder :-).

Adrian. 

Sent from my iPad

On Aug 12, 2010, at 8:06 AM, Glenn Adams <gl...@skynav.com> wrote:

> 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