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
