I am in favor of clean code. You can live with code that has warnings as long as it compiles, runs, and doesn't crash, but it can be annoying to other developers and should surely be removed by the time you're calling it 1.0. Important warnings like calls to deprecated methods and dead code should be cleaned up. Code should not give insificant warnings like an arbitrary maximum line length. Surely it's bad practice to make lines unnecessarily long if they can reasonably be broken up, but it is often easier and possibly more efficient for someone to code a line longer than you would prefer. Fleas on the couch should make the couch unusable. Those would be your errors. Warnings are the stains. You can sit on a stained couch with no harm to yourself, though others would see your couch as less than desirable, and if everyone allows new stains it would make your couch so ugly that only the most desperate would bother to use it and they would likely cringe in doing so. I was a bit puzzled and annoyed when I first tried to get the source and test an update, wondering why anyone would publish code with so many reported problems, though I chose to ignore the problem as others apparently had done since it seems to be the best program available to do what I wanted to do and it worked despite the problems.
________________________________ From: Glenn Adams [mailto:gl...@skynav.com] Sent: Wednesday, August 11, 2010 8:06 PM To: fop-dev@xmlgraphics.apache.org Subject: Re: [Bug 49733] [PATCH] resolve compilation, checkstyle, javadoc warnings 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
