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