RE: [Bug 49733] [PATCH] resolve compilation, checkstyle, javadoc warnings
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 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. Inst
Re: [Bug 49733] [PATCH] resolve compilation, checkstyle, javadoc warnings (a proposal for next steps)
Jeremias Maerki wrote: Hi All, I've mentioned the deprecated methods in my reply to Vincent. I'll restore two instances which can be handled later. At least one is kind of important as long as I haven't done a Barcode4J 2.1 release (which is long overdue). Do I understand you correctly, Simon, that you're OK to leave the CS comments for now and revisit later? I could live with that, too. If I read Vincent and Chris correctly, they are not absolutely against having the CS comments for now although they are not at all happy about them. Please correct me if I got that wrong! Removing them later is always a possibility. I'm not too happy to disregard a majority opinion especially since Glenn is not yet a committer. But I guess leaving the CS comments for now allows us to continue and we can still reduce (or get rid of) the CS comments later. I think you understood me correctly. Whilst I prefer not to have lots of CS comments scattered around I also don't want to stop Glenn working. My preferred option from the list of 3 is #2, but as Glenn already indicated that is unacceptable to him I'm happy to vote -0 here. I know I'm currently behaving like a flag in the wind but I'm really a bit clueless what the best way is since we do not have a consensus right now. But I'd like to continue here as quickly as possible. I didn't get to handle the patch today due to a support request (FOP go boom with PDF sizes over 2GB). But the weather doesn't look to good here during the weekend so I may be able to get this done tomorrow. Thanks, Chris On 13.08.2010 14:40:55 Simon Pepping wrote: Glenn, On Fri, Aug 13, 2010 at 05:07:52PM +0800, Glenn Adams wrote: In any case, we now appear to be at a juncture where one of the following options may be implemented: (1) leave the CS* comments in place, but DON'T change the checkstyle rules AT THIS TIME (but reserve option to change later) (2) remove the CS* comments, but DON'T change the checkstyle rules, leaving at least 279 warnings/errors to be produced; (3) remove the CS* comments, but DO change the checkstyle rules AT THIS TIME such that none of the CS* comments are required I prefer option #1. I cannot accept option #2, since it leaves a large number of reported warnings, thus negating my primary goal in creating this patch. I can live with option #3, although it requires editing around 100 files to remove the CS* comments. And it also requires modifying the checkstyle rule set, and in some cases removing or weakening potentially useful rules. I would prefer something like option #2, and so do a few other committers. I understand this produces an unacceptable working mode for you. I can live with that, and we can review the CHECKSTYLE comments later in an effort to make further improvements. I would like to hear Jeremias' comment on the removal of the deprecated methods. Deprecated methods are a fact of life. Simon -- Simon Pepping home page: http://www.leverkruid.eu Jeremias Maerki
Re: [Bug 49733] [PATCH] resolve compilation, checkstyle, javadoc warnings (a proposal for next steps)
Thanks for providing your view. So we have a similar view. This gives me another shove in the butt to finally make the B4J 2.1 release. JEuclid should already be fine. I've seen that Max has already switched to the other method. So, we can easily remove the deprecate method before the next release but not really before I've fixed B4J as people who work with FOP Trunk would run into problems. On 13.08.2010 17:22:26 Simon Pepping wrote: > I want to move forward with Glenn's work. As you wrote, it is an > important addition to FOP which we cannot let go unused. > > I feel that the CHECKSTYLE comments are clear, and allow us to take > any action later that we require. They could be harmful if we would > feel that further work would have to be done, but actually is not > done. Then the comments will hide that situation. But that is up to > us. > > I understand that Glenn wants to work from a clean checkstyle and > javadoc situation, which allows him to have a clear view on the > consequences of his own actions. It is a fairly puristic stand point, > but I appreciate that he feels that he needs it to do a good > job. After all, his is quite a far-reaching code change. > > We do need to retain a few deprecated methods, as they are deprecated > parts of the public API, of which you are one of the users. It would > be good if we could document the new alternatives, and fix a time when > they can be removed. Perhaps at release 1.1, as they were already > deprecated at release 1.0? > > We will see how the other team members stand in this issue, but I will > be thoroughly disappointed if we cannot move forward efficiently with > this work. > > Simon > > On Fri, Aug 13, 2010 at 04:47:07PM +0200, Jeremias Maerki wrote: > > I've mentioned the deprecated methods in my reply to Vincent. I'll > > restore two instances which can be handled later. At least one is kind > > of important as long as I haven't done a Barcode4J 2.1 release (which is > > long overdue). > > > > Do I understand you correctly, Simon, that you're OK to leave the CS > > comments for now and revisit later? I could live with that, too. If I > > read Vincent and Chris correctly, they are not absolutely against having > > the CS comments for now although they are not at all happy about them. > > Please correct me if I got that wrong! Removing them later is always a > > possibility. I'm not too happy to disregard a majority opinion > > especially since Glenn is not yet a committer. But I guess leaving the > > CS comments for now allows us to continue and we can still reduce (or > > get rid of) the CS comments later. > > > > I know I'm currently behaving like a flag in the wind but I'm really a > > bit clueless what the best way is since we do not have a consensus right > > now. But I'd like to continue here as quickly as possible. I didn't get > > to handle the patch today due to a support request (FOP go boom with PDF > > sizes over 2GB). But the weather doesn't look to good here during the > > weekend so I may be able to get this done tomorrow. > > > > On 13.08.2010 14:40:55 Simon Pepping wrote: > > > Glenn, > > > > > > On Fri, Aug 13, 2010 at 05:07:52PM +0800, Glenn Adams wrote: > > > > In any case, we now appear to be at a juncture where one of the > > > > following > > > > options may be implemented: > > > > > > > > (1) leave the CS* comments in place, but DON'T change the checkstyle > > > > rules > > > > AT THIS TIME (but reserve option to change later) > > > > (2) remove the CS* comments, but DON'T change the checkstyle rules, > > > > leaving > > > > at least 279 warnings/errors to be produced; > > > > (3) remove the CS* comments, but DO change the checkstyle rules AT THIS > > > > TIME > > > > such that none of the CS* comments are required > > > > > > > > I prefer option #1. > > > > > > > > I cannot accept option #2, since it leaves a large number of reported > > > > warnings, thus negating my primary goal in creating this patch. > > > > > > > > I can live with option #3, although it requires editing around 100 > > > > files to > > > > remove the CS* comments. And it also requires modifying the checkstyle > > > > rule > > > > set, and in some cases removing or weakening potentially useful rules. > > > > > > I would prefer something like option #2, and so do a few other > > > committers. I understand this produces an unacceptable working mode > > > for you. I can live with that, and we can review the CHECKSTYLE > > > comments later in an effort to make further improvements. > > > > > > I would like to hear Jeremias' comment on the removal of the > > > deprecated methods. Deprecated methods are a fact of life. > > -- > Simon Pepping > home page: http://www.leverkruid.eu Jeremias Maerki
Re: [Bug 49733] [PATCH] resolve compilation, checkstyle, javadoc warnings (a proposal for next steps)
I want to move forward with Glenn's work. As you wrote, it is an important addition to FOP which we cannot let go unused. I feel that the CHECKSTYLE comments are clear, and allow us to take any action later that we require. They could be harmful if we would feel that further work would have to be done, but actually is not done. Then the comments will hide that situation. But that is up to us. I understand that Glenn wants to work from a clean checkstyle and javadoc situation, which allows him to have a clear view on the consequences of his own actions. It is a fairly puristic stand point, but I appreciate that he feels that he needs it to do a good job. After all, his is quite a far-reaching code change. We do need to retain a few deprecated methods, as they are deprecated parts of the public API, of which you are one of the users. It would be good if we could document the new alternatives, and fix a time when they can be removed. Perhaps at release 1.1, as they were already deprecated at release 1.0? We will see how the other team members stand in this issue, but I will be thoroughly disappointed if we cannot move forward efficiently with this work. Simon On Fri, Aug 13, 2010 at 04:47:07PM +0200, Jeremias Maerki wrote: > I've mentioned the deprecated methods in my reply to Vincent. I'll > restore two instances which can be handled later. At least one is kind > of important as long as I haven't done a Barcode4J 2.1 release (which is > long overdue). > > Do I understand you correctly, Simon, that you're OK to leave the CS > comments for now and revisit later? I could live with that, too. If I > read Vincent and Chris correctly, they are not absolutely against having > the CS comments for now although they are not at all happy about them. > Please correct me if I got that wrong! Removing them later is always a > possibility. I'm not too happy to disregard a majority opinion > especially since Glenn is not yet a committer. But I guess leaving the > CS comments for now allows us to continue and we can still reduce (or > get rid of) the CS comments later. > > I know I'm currently behaving like a flag in the wind but I'm really a > bit clueless what the best way is since we do not have a consensus right > now. But I'd like to continue here as quickly as possible. I didn't get > to handle the patch today due to a support request (FOP go boom with PDF > sizes over 2GB). But the weather doesn't look to good here during the > weekend so I may be able to get this done tomorrow. > > On 13.08.2010 14:40:55 Simon Pepping wrote: > > Glenn, > > > > On Fri, Aug 13, 2010 at 05:07:52PM +0800, Glenn Adams wrote: > > > In any case, we now appear to be at a juncture where one of the following > > > options may be implemented: > > > > > > (1) leave the CS* comments in place, but DON'T change the checkstyle rules > > > AT THIS TIME (but reserve option to change later) > > > (2) remove the CS* comments, but DON'T change the checkstyle rules, > > > leaving > > > at least 279 warnings/errors to be produced; > > > (3) remove the CS* comments, but DO change the checkstyle rules AT THIS > > > TIME > > > such that none of the CS* comments are required > > > > > > I prefer option #1. > > > > > > I cannot accept option #2, since it leaves a large number of reported > > > warnings, thus negating my primary goal in creating this patch. > > > > > > I can live with option #3, although it requires editing around 100 files > > > to > > > remove the CS* comments. And it also requires modifying the checkstyle > > > rule > > > set, and in some cases removing or weakening potentially useful rules. > > > > I would prefer something like option #2, and so do a few other > > committers. I understand this produces an unacceptable working mode > > for you. I can live with that, and we can review the CHECKSTYLE > > comments later in an effort to make further improvements. > > > > I would like to hear Jeremias' comment on the removal of the > > deprecated methods. Deprecated methods are a fact of life. -- Simon Pepping home page: http://www.leverkruid.eu
Re: [Bug 49733] [PATCH] resolve compilation, checkstyle, javadoc warnings (a proposal for next steps)
I've mentioned the deprecated methods in my reply to Vincent. I'll restore two instances which can be handled later. At least one is kind of important as long as I haven't done a Barcode4J 2.1 release (which is long overdue). Do I understand you correctly, Simon, that you're OK to leave the CS comments for now and revisit later? I could live with that, too. If I read Vincent and Chris correctly, they are not absolutely against having the CS comments for now although they are not at all happy about them. Please correct me if I got that wrong! Removing them later is always a possibility. I'm not too happy to disregard a majority opinion especially since Glenn is not yet a committer. But I guess leaving the CS comments for now allows us to continue and we can still reduce (or get rid of) the CS comments later. I know I'm currently behaving like a flag in the wind but I'm really a bit clueless what the best way is since we do not have a consensus right now. But I'd like to continue here as quickly as possible. I didn't get to handle the patch today due to a support request (FOP go boom with PDF sizes over 2GB). But the weather doesn't look to good here during the weekend so I may be able to get this done tomorrow. On 13.08.2010 14:40:55 Simon Pepping wrote: > Glenn, > > On Fri, Aug 13, 2010 at 05:07:52PM +0800, Glenn Adams wrote: > > In any case, we now appear to be at a juncture where one of the following > > options may be implemented: > > > > (1) leave the CS* comments in place, but DON'T change the checkstyle rules > > AT THIS TIME (but reserve option to change later) > > (2) remove the CS* comments, but DON'T change the checkstyle rules, leaving > > at least 279 warnings/errors to be produced; > > (3) remove the CS* comments, but DO change the checkstyle rules AT THIS TIME > > such that none of the CS* comments are required > > > > I prefer option #1. > > > > I cannot accept option #2, since it leaves a large number of reported > > warnings, thus negating my primary goal in creating this patch. > > > > I can live with option #3, although it requires editing around 100 files to > > remove the CS* comments. And it also requires modifying the checkstyle rule > > set, and in some cases removing or weakening potentially useful rules. > > I would prefer something like option #2, and so do a few other > committers. I understand this produces an unacceptable working mode > for you. I can live with that, and we can review the CHECKSTYLE > comments later in an effort to make further improvements. > > I would like to hear Jeremias' comment on the removal of the > deprecated methods. Deprecated methods are a fact of life. > > Simon > > -- > Simon Pepping > home page: http://www.leverkruid.eu Jeremias Maerki
Re: [Bug 49733] [PATCH] resolve compilation, checkstyle, javadoc warnings (a proposal for next steps)
Glenn, On Fri, Aug 13, 2010 at 05:07:52PM +0800, Glenn Adams wrote: > In any case, we now appear to be at a juncture where one of the following > options may be implemented: > > (1) leave the CS* comments in place, but DON'T change the checkstyle rules > AT THIS TIME (but reserve option to change later) > (2) remove the CS* comments, but DON'T change the checkstyle rules, leaving > at least 279 warnings/errors to be produced; > (3) remove the CS* comments, but DO change the checkstyle rules AT THIS TIME > such that none of the CS* comments are required > > I prefer option #1. > > I cannot accept option #2, since it leaves a large number of reported > warnings, thus negating my primary goal in creating this patch. > > I can live with option #3, although it requires editing around 100 files to > remove the CS* comments. And it also requires modifying the checkstyle rule > set, and in some cases removing or weakening potentially useful rules. I would prefer something like option #2, and so do a few other committers. I understand this produces an unacceptable working mode for you. I can live with that, and we can review the CHECKSTYLE comments later in an effort to make further improvements. I would like to hear Jeremias' comment on the removal of the deprecated methods. Deprecated methods are a fact of life. Simon -- Simon Pepping home page: http://www.leverkruid.eu
Re: [Bug 49733] [PATCH] resolve compilation, checkstyle, javadoc warnings (a proposal for next steps)
On Fri, Aug 13, 2010 at 4:32 PM, Simon Pepping wrote: > > Glenn notes that he used comments to suppress checkstyle warnings in > such cases as: > > - certain uses of package, protected, or public visibility of fields, > which would have required a fairly large number of changes to substitute > calls to new getX() or setX(...) methods; > > Leaving the warnings would be a sign that some work is to be > done. Suppressing the warnings gives the false idea that no more work > is to be done, and that all comments represent a sound judgment to > leave the code intentionally as is. > > Otherwise I am in favour of using warnings to mark code that > intentionally does not comply with the rules, at the judgment of the > developer. The primary reason I undertook this cleanup work is because my complex scripts patch touched so many core files that already contained warnings. In modifying them, I wished to ensure that I did not introduce new warnings, however determining this was quite painful and time consuming given the >2000 warnings that existed. In this state, it is very easy to allow new, unintentional errors to creep in, or to ignore this form of testing altogether. I think this has been the status quo, and is probably responsible for the existence of so many warnings. On the other hand, if there are no warnings during build, javadoc build, checkstyle, etc., then it is easy for a developer to notice and fix problems before they become unmanageable. That was my goal in this patch, to create a noiseless baseline. As the process proceeded, I found I could address the majority of warnings/errors directly, without resorting to warning suppression. However, as I've already pointed out, this was not always possible or would have been impractical or potentially destabilizing to implement the necessary changes. In these cases, inline suppressions did the trick. Further, their existence left in place an easy mechanism for learning of, tracking, and subsequently applying more in-depth fixes. A mere grep of the code easily shows where the were used, and, in fact, it would be easy enough to add a build target that produces a report of their presence. Furthermore, I did not want to undertake at this time a discussion (argument?) about what is good style or bad style, what should be always enforced or what should be merely a guideline. I continue to be reluctant to have such a discussion, partly because I think it is a waste of time that could be applied to other more useful work. In any case, we now appear to be at a juncture where one of the following options may be implemented: (1) leave the CS* comments in place, but DON'T change the checkstyle rules AT THIS TIME (but reserve option to change later) (2) remove the CS* comments, but DON'T change the checkstyle rules, leaving at least 279 warnings/errors to be produced; (3) remove the CS* comments, but DO change the checkstyle rules AT THIS TIME such that none of the CS* comments are required I prefer option #1. I cannot accept option #2, since it leaves a large number of reported warnings, thus negating my primary goal in creating this patch. I can live with option #3, although it requires editing around 100 files to remove the CS* comments. And it also requires modifying the checkstyle rule set, and in some cases removing or weakening potentially useful rules. G.
Re: [Bug 49733] [PATCH] resolve compilation, checkstyle, javadoc warnings (a proposal for next steps)
On Thu, Aug 12, 2010 at 02:25:33PM +0200, Jeremias Maerki wrote: Hi, I have returned and read the discussions. I have the following remarks: Building fop with jdk 1.4, as required, gives an error when checkstyle-all-5.0.jar is present. The major.minor version 49.0 is not supported. Thus removing checkstyle-4.0 gives an inconsistency between the development environment and the release build environment, which is workable but at the same time annoying. Some methods marked deprecated were part of our (unofficial) API. Deprecation requires some time in which application builders can change over. Indeed, a description of the alternative and a time frame to change over would have been useful. If we remove these methods, we must be prepared to face application builders at our next release. Glenn notes that he used comments to suppress checkstyle warnings in such cases as: - certain uses of package, protected, or public visibility of fields, which would have required a fairly large number of changes to substitute calls to new getX() or setX(...) methods; Leaving the warnings would be a sign that some work is to be done. Suppressing the warnings gives the false idea that no more work is to be done, and that all comments represent a sound judgment to leave the code intentionally as is. Otherwise I am in favour of using warnings to mark code that intentionally does not comply with the rules, at the judgment of the developer. > I would suggest the following as our next steps: > > 1. Clarify the thing with LineBreak*. > 2. Decide (quickly, please) whether to remove the //CS comments or to > allow them for now and optionally do something about them later. (I'm > tending towards removing them but I don't have a problem if we do it the > other way.) > 3. Commit the patch to Trunk more or less as is (pending //CS decision). > 3. Adjust the Checkstyle profile to allow "log" and disallow whitespace > before and after parantheses. Then remove "log"-related //CS constants > and excessive whitespace. > 4. Merge the changes into the Temp_ComplexScripts parts. > 5. Glenn could then provide a new patch against the branch which we > could do a cursory review on. We apply that and experiment with what > he's built. He can continue his work. > 6. We continue to incrementally improve our coding standards. > > I'm happy to do the grunt work. Like Glenn, I don't like to hold > principle discussions right now because that holds up several people > from doing day-to-day work. That doesn't mean we can't hold them, but I > don't see why we have to do it as a precondition to processing this > patch. The patch gets us further but doesn't preclude any futher > improvements later. > > Please, let's get this done. Generally I agree with this plan. Specifically, I do not want to wait for future discussions about better rules. That would make better the enemy of good. I want to take the practical approach that the current work is an improvement over what we had, and must be applied after concensus over the above discussed points. Simon -- Simon Pepping home page: http://www.leverkruid.eu
Re: [Bug 49733] [PATCH] resolve compilation, checkstyle, javadoc warnings (a proposal for next steps)
On Fri, Aug 13, 2010 at 10:57 AM, Glenn Adams wrote: > 3. Adjust the Checkstyle profile to allow "log" and disallow whitespace >> before and after parantheses. Then remove "log"-related //CS constants >> and excessive whitespace. >> > > I would not agree to restricting the style rules to prohibit whitespace > before/after parentheses. I prefer *always* using whitespace around parens > in Java (and C/C++). > Allow me to expand on this. I can accept such a rule (for prohibiting whitespace before/after parens), but only if it is accepted that CSOFF be used to disable that rule globally in the files of which I am the original author. That is, I could agree to enforce and use that rule on files I did not author, as long as I can avoid using that rule on the files I author. By the way, this is precisely why there are going to always be limits to obtaining consensus on style rules, particularly on those that are the most stylistic in nature, of which I would suggest that whitespace distribution will remain the most subjective. What to do in such a case? Either don't impose the rule at all, or impose it as a default while allowing overrides for those that do not concur. There may indeed be some core set of rules where there is a true consensus on application and enforcing, some of which may be related to whitespace. For example, should tabs be permitted? Even though my editor can handle that with appropriate embedded comments in the code, it is undesirable, and not everyone uses the same editor. So best stay with NO TABS. On the other hand, there are other possible rules for which a unanimous consensus will be impossible, and for those, we can only not employ the rule or employ it *merely* as a default, allowing overrides. You will also now notice that we have descended onto that slippery slope of discussing subjective preferences about style rules. I hope we can climb off that slope soon and finish this patch in order to progress with useful features. G.
Re: [Bug 49733] [PATCH] resolve compilation, checkstyle, javadoc warnings (a proposal for next steps)
inline
On Thu, Aug 12, 2010 at 8:25 PM, Jeremias Maerki wrote:
>
> 1. Clarify the thing with LineBreak*.
>
It was necessary to update the line break data in order to regenerate
LineBreakUtils.java; otherwise, the generation process failed to to a
missing column ('CP') when it attempts to pull down and reprocess the
Unicode property data.
> 2. Decide (quickly, please) whether to remove the //CS comments or to
> allow them for now and optionally do something about them later. (I'm
> tending towards removing them but I don't have a problem if we do it the
> other way.)
>
Removing them wastes the effort I made to track them down and make a
judgment call over each one. If you want to remove them, then I want a line
by line review of every one being removed, with justification for its
removal.
Removing them allows at least 279 warnings to remain, which is 279 too many.
The presence of more than zero warnings tells other committers and
developers that increasing the number of warnings is tolerated and accepted,
which is not a good message to send.
Removing them yields to the wrong idea that they should not be use, and that
there should be no exceptions made to the style rules. That is wrong
thinking in my opinion.
Having said the above, I would agree with removing them if the checkstyle
rules are changed so that their removal does not cause any warnings. That
would mean removing the following checks or adjusting them so that they were
not triggered:
ConstantNameCheck
FileLengthCheck
InnerAssiignmentCheck
LineLengthCheck
ParameterNumberCheck
MethodLengthCheck
VisibilityModifierCheck
I sent the full list out yesterday, but I have not seen any comments on
specifics. If this patch is going to be delayed to resolve this NOW instead
of gradually over time (the wiser choice) then, we need to review them all
and sign off on them.
I cannot accept a result that produces any warning output.
> 3. Commit the patch to Trunk more or less as is (pending //CS decision).
> 3. Adjust the Checkstyle profile to allow "log" and disallow whitespace
> before and after parantheses. Then remove "log"-related //CS constants
> and excessive whitespace.
>
I would not agree to restricting the style rules to prohibit whitespace
before/after parentheses. I prefer *always* using whitespace around parens
in Java (and C/C++).
> 4. Merge the changes into the Temp_ComplexScripts parts.
>
Yes, whatever the outcome of the warning cleanup, it should be merged into
that branch before applying the complex scripts patch. I will update that
patch once the cleanup merge occurs so that there are no merge problems,
then the updated patch can be used to populate that branch.
> 5. Glenn could then provide a new patch against the branch which we
> could do a cursory review on. We apply that and experiment with what
> he's built. He can continue his work.
>
Yes.
> 6. We continue to incrementally improve our coding standards.
Unfortunately, IMO, the increment being proposed here is larger than
necessary. The patch could be applied while leaving the CS* comments in
place without doing any damage to quality, and, in fact, improves quality by
recording the result of a careful audit. The group *could* if it chooses,
subsequently make incremental improvements to fine-tune the style rules and
removing some or even most of the CS* comments, but I reject the notion that
no CS suppressions should ever be used. That is an attempt to impose an
ideal to coding style for which we will not be able to obtain an absolute
consensus. The options are only as follows:
1. ignore warnings - which has been the status quo, and what we are
trying to fix here
2. choose such a loose set of style rules such that no warnings occur,
which may mean reducing the effectiveness of the tool and process;
3. take a pragmatic stance, using tighter rules but allow developers to
use common sense and intelligence in using CS* comments;
Of these options, #1 says do nothing, thus resulting in a waste of my time
and this exercise, and goes against the apparent wishes of most who have
commented positively on fixing this. Option #2 may allow achieving the goal
of zero warnings, but may prevent catching cases where some coding change is
warranted, e.g., catching field visibility issues. Option #3 permits the use
of tighter rules to catch potential problems, while giving developers the
tools they need to make informed choices about when to go outside of the
style guidelines.
My vote is #3.
G.
Re: [Bug 49733] [PATCH] resolve compilation, checkstyle, javadoc warnings (a proposal for next steps)
Thanks, Vincent. On 12.08.2010 18:31:50 Vincent Hennebert wrote: > Hi, > > Jeremias Maerki wrote: > > I've now applied the patch locally and done a detailed review. I'm > > posting this a bit outside the context of recent discussions to simply > > state my present opinion after looking into the patch. > > > > Generally, this is a big improvement. So thanks, Glenn, for your work > > here! > > > > I'm also not particularly happy about the //CS* comments. To a certain > > degree I think I could live with them. A count shows 279 usages. I think > > that may be a tad too much. Maybe we can find something in between, like > > making more use of the "error" severity. Most checks are just warnings > > right now. So using errors will make it easier to enforce at least the > > rules most important to us. I've also experimented with the regular > > expressions: > > > > > > > > > > > > > > This should already make several such //CS comments unnecessary. There > > are other comments referencing ConstantNameCheck where we should rather > > convert the name to upper case. That will cut down on these even further. > > Like Chris suggested, we could then even decide to live with a few > > warnings as long as we increase the severity of the most important rules > > and set up a no-go policy for "errors". > > (This is precisely why I suggested that we agree on an improved > Checkstyle first, to avoid introducing unnecessary //CS comments.) You sounded like you wanted to discuss this forwards and backwards before going through with the patch. We only need to agree on some key aspects and can deal with the finer points later. > I don’t really have an opinion about that. Since zero-warning won’t be > achieved in the short term anyway, I suppose we could remove them for > now. Once we decide to enforce a zero-warning policy then they will > probably have to be used, along with a TODO warning indicated that this > is old code that needs refactoring; and thus make the difference with > new CSOK comments introduced later on with due care. So, if I get this right, we have one person (Glenn) for the //CS comments, and 3 people (Vincent, Chris and me) more or less against them. Glenn, I hope you can live with it if I remove them when processing the patch tomorrow. I'll try to get rid of some of the issues in a second step so we get the warnings down to a "comfortable" level. > > I saw some changes in LineBreakPairTable.txt and LineBreakUtils.java. > > Glenn, was that an accidental overflow from your work on the new > > features? > > > > I have no problem with the sometimes rather generic Javadoc comments. > > Every committer is invited to improve on those as he's passing over > > particular code parts. I know that we have quite a bit of outdated > > documentation already in our Javadocs. So these comments don't make the > > situation worse IMO. The only thing we can do is gradually improve. But > > at least the generic javadocs lets us cut down on the number of warnings > > so we can really focus to improve there rather than capitulate before > > thousands of warnings. > > I’m ok with that. Some less generic comments will need to be > double-checked. Not that I don’t trust Glenn on that matter, but some > parts of the code (especially layout) are tricky and it’s very easy to > be mistaken. And I think a wrong Javadoc comment does more harm than no > comment at all. ...if they are ever read at all. ;-) But you're right essentially. As I said: this will always be work/improvements in progress. > > Finally a nit: some files have got method signatures with whitespace > > before and after the parantheses. We don't traditionally do that but the > > Checkstyle profile doesn't seem to catch that. I guess it would be safe > > to add that rule so we can fix those occurences. > > +1 > > > > I would suggest the following as our next steps: > > > > 1. Clarify the thing with LineBreak*. > > 2. Decide (quickly, please) whether to remove the //CS comments or to > > allow them for now and optionally do something about them later. (I'm > > tending towards removing them but I don't have a problem if we do it the > > other way.) > > +1 for removing them for now. > > > > 3. Commit the patch to Trunk more or less as is (pending //CS decision). > > -1, among other things there are deprecated flags/methods that were > removed and I feel that that must be discussed first (mainly > Graphics2DAdapter.paintImage). Sorry, missed that. I've still not changed Barcode4J so the current B4J release would probably fail if this method is removed. I guess I should finally fix that. Renderer.startPageSequence(LineArea) can probably safely be removed (Deprecation since Jan 2008). But we can leave that to decide for later. > > 3. Adjust the Checkstyle profile to allow "log" and disallow whitespace > > before and after parantheses. Then remove "log"-related //CS constants > > and excessive whitespace. > > +0, I would just put
Re: [Bug 49733] [PATCH] resolve compilation, checkstyle, javadoc warnings (a proposal for next steps)
Hi, Jeremias Maerki wrote: > I've now applied the patch locally and done a detailed review. I'm > posting this a bit outside the context of recent discussions to simply > state my present opinion after looking into the patch. > > Generally, this is a big improvement. So thanks, Glenn, for your work > here! > > I'm also not particularly happy about the //CS* comments. To a certain > degree I think I could live with them. A count shows 279 usages. I think > that may be a tad too much. Maybe we can find something in between, like > making more use of the "error" severity. Most checks are just warnings > right now. So using errors will make it easier to enforce at least the > rules most important to us. I've also experimented with the regular > expressions: > > > > > > > This should already make several such //CS comments unnecessary. There > are other comments referencing ConstantNameCheck where we should rather > convert the name to upper case. That will cut down on these even further. > Like Chris suggested, we could then even decide to live with a few > warnings as long as we increase the severity of the most important rules > and set up a no-go policy for "errors". (This is precisely why I suggested that we agree on an improved Checkstyle first, to avoid introducing unnecessary //CS comments.) I don’t really have an opinion about that. Since zero-warning won’t be achieved in the short term anyway, I suppose we could remove them for now. Once we decide to enforce a zero-warning policy then they will probably have to be used, along with a TODO warning indicated that this is old code that needs refactoring; and thus make the difference with new CSOK comments introduced later on with due care. > I saw some changes in LineBreakPairTable.txt and LineBreakUtils.java. > Glenn, was that an accidental overflow from your work on the new > features? > > I have no problem with the sometimes rather generic Javadoc comments. > Every committer is invited to improve on those as he's passing over > particular code parts. I know that we have quite a bit of outdated > documentation already in our Javadocs. So these comments don't make the > situation worse IMO. The only thing we can do is gradually improve. But > at least the generic javadocs lets us cut down on the number of warnings > so we can really focus to improve there rather than capitulate before > thousands of warnings. I’m ok with that. Some less generic comments will need to be double-checked. Not that I don’t trust Glenn on that matter, but some parts of the code (especially layout) are tricky and it’s very easy to be mistaken. And I think a wrong Javadoc comment does more harm than no comment at all. > Finally a nit: some files have got method signatures with whitespace > before and after the parantheses. We don't traditionally do that but the > Checkstyle profile doesn't seem to catch that. I guess it would be safe > to add that rule so we can fix those occurences. +1 > I would suggest the following as our next steps: > > 1. Clarify the thing with LineBreak*. > 2. Decide (quickly, please) whether to remove the //CS comments or to > allow them for now and optionally do something about them later. (I'm > tending towards removing them but I don't have a problem if we do it the > other way.) +1 for removing them for now. > 3. Commit the patch to Trunk more or less as is (pending //CS decision). -1, among other things there are deprecated flags/methods that were removed and I feel that that must be discussed first (mainly Graphics2DAdapter.paintImage). > 3. Adjust the Checkstyle profile to allow "log" and disallow whitespace > before and after parantheses. Then remove "log"-related //CS constants > and excessive whitespace. +0, I would just put log in uppercase but I don’t really mind. > 4. Merge the changes into the Temp_ComplexScripts parts. > 5. Glenn could then provide a new patch against the branch which we > could do a cursory review on. We apply that and experiment with what > he's built. He can continue his work. > 6. We continue to incrementally improve our coding standards. > > I'm happy to do the grunt work. Like Glenn, I don't like to hold > principle discussions right now because that holds up several people > from doing day-to-day work. > That doesn't mean we can't hold them, but I > don't see why we have to do it as a precondition to processing this > patch. The patch gets us further but doesn't preclude any futher > improvements later. > > Please, let's get this done. I’m not happy with that approach. When this topic was first mentioned [1] I did say that the Checkstyle file needed improvement and that until then this would be premature to work on that. My advice was not followed and now we should apply this patch ASAP without discussion? I’m not sure that trying to force things is a good way to get involved. The consensus-based approach inherent to any Apache project is not being followed here. [
Re: [Bug 49733] [PATCH] resolve compilation, checkstyle, javadoc warnings (a proposal for next steps)
I've now applied the patch locally and done a detailed review. I'm posting this a bit outside the context of recent discussions to simply state my present opinion after looking into the patch. Generally, this is a big improvement. So thanks, Glenn, for your work here! I'm also not particularly happy about the //CS* comments. To a certain degree I think I could live with them. A count shows 279 usages. I think that may be a tad too much. Maybe we can find something in between, like making more use of the "error" severity. Most checks are just warnings right now. So using errors will make it easier to enforce at least the rules most important to us. I've also experimented with the regular expressions: This should already make several such //CS comments unnecessary. There are other comments referencing ConstantNameCheck where we should rather convert the name to upper case. That will cut down on these even further. Like Chris suggested, we could then even decide to live with a few warnings as long as we increase the severity of the most important rules and set up a no-go policy for "errors". I saw some changes in LineBreakPairTable.txt and LineBreakUtils.java. Glenn, was that an accidental overflow from your work on the new features? I have no problem with the sometimes rather generic Javadoc comments. Every committer is invited to improve on those as he's passing over particular code parts. I know that we have quite a bit of outdated documentation already in our Javadocs. So these comments don't make the situation worse IMO. The only thing we can do is gradually improve. But at least the generic javadocs lets us cut down on the number of warnings so we can really focus to improve there rather than capitulate before thousands of warnings. Finally a nit: some files have got method signatures with whitespace before and after the parantheses. We don't traditionally do that but the Checkstyle profile doesn't seem to catch that. I guess it would be safe to add that rule so we can fix those occurences. I would suggest the following as our next steps: 1. Clarify the thing with LineBreak*. 2. Decide (quickly, please) whether to remove the //CS comments or to allow them for now and optionally do something about them later. (I'm tending towards removing them but I don't have a problem if we do it the other way.) 3. Commit the patch to Trunk more or less as is (pending //CS decision). 3. Adjust the Checkstyle profile to allow "log" and disallow whitespace before and after parantheses. Then remove "log"-related //CS constants and excessive whitespace. 4. Merge the changes into the Temp_ComplexScripts parts. 5. Glenn could then provide a new patch against the branch which we could do a cursory review on. We apply that and experiment with what he's built. He can continue his work. 6. We continue to incrementally improve our coding standards. I'm happy to do the grunt work. Like Glenn, I don't like to hold principle discussions right now because that holds up several people from doing day-to-day work. That doesn't mean we can't hold them, but I don't see why we have to do it as a precondition to processing this patch. The patch gets us further but doesn't preclude any futher improvements later. Please, let's get this done. Jeremias Maerki
Re: [Bug 49733] [PATCH] resolve compilation, checkstyle, javadoc warnings
vincent, my apologies if I offended, as that was not my intent; i'm sure we will manage to work our way through this towards a successful conclusion; respectfully, glenn On Thu, Aug 12, 2010 at 7:41 PM, Vincent Hennebert wrote: > This message lacks of courtesy, therefore I do not wish to continue the > discussion. >
Re: [Bug 49733] [PATCH] resolve compilation, checkstyle, javadoc warnings
This message lacks of courtesy, therefore I do not wish to continue the discussion. I’ll proceed as I explained in my previous message. Or maybe it’s just me not being a native English speaker... Vincent Glenn Adams wrote: > Inline below. > > On Wed, Aug 11, 2010 at 7:45 PM, Vincent Hennebert > 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
Re: [Bug 49733] [PATCH] resolve compilation, checkstyle, javadoc warnings
On Thu, Aug 12, 2010 at 3:18 PM, Glenn Adams wrote: > that is a reasonable objection to removing the file, so indeed i would not > object to leaving it in place; however, i did not test checkstyle 1.4, and > indeed, there are a few changes that appear at the end of the new > checkstyles-5.1.xml file which would need to be added to the older > checkstyle-1.4.xml file (presuming they are supported in 1.4) in order to > enable the currently used suppressions; on the other hand, i'm not sure we > want to make (or continue to make) FOP compatible with specific IDEs, since > at present, only the command line ANT build is effectively supported; but in > the interest of compromise, i won't object to retaining the old 1.4 file > (you may wish to update it with the new suppression clauses), namely, by > adding: > s/1.4/4.0/g
Re: [Bug 49733] [PATCH] resolve compilation, checkstyle, javadoc warnings
@ Chris On Wed, Aug 11, 2010 at 11:20 PM, Chris Bowditch wrote: > > First of all let me start by saying many thanks to Glenn for the hours and > hours of effort that he must of put into going through the code and > resolving the checkstyle warnings. Thanks Glenn! > > Your welcome. I logged ~40 hours of real time doing the work on bugs 49700, 49703, and 49733, so I certainly don't want to see it wasted. Further, I don't want to continue to have to deal with these warnings while I work on the complex scripts features, since it touches so many files. I agree with Vincent here. I don't like the code being cluttered with CSOFF > comments and it does defeat the purpose of checkstyle. Why not just accept > that there will still be a few warnings left after the patch is applied. The > patch is still a huge improvement on the current situation and we can try to > address the remaining few warnings over time. I don't believe I used any CSOFF comments in the patch, however, I did use a number of CSOK comments, which disable for a specific line only. Over time, we should be able to eliminate these, but they were important to deal with certain reported errors, otherwise the alternative was to rewrite the checkstyle rules, which I did not want to do. Some examples of when CSOK was used: - parameter count greater than 7 - method length greater than 150 lines - certain static final fields, which would be flagged by ConstantNameCheck for not being all upper case, but which by convention use lower case, e.g., the "log" field, which typically is (or should be) defined as "private static final Log log = LogFactor.getLog(...)" - certain uses of an inner assignment, particularly in multi clause if/then statements, where some conditional expression contains an inner assignment whose resulting side effects are used by subsequently evaluated sub-expressions; although I rewrote the code to remove some of these, in other cases it would have been to much refactoring, so I left it as is with a CSOK to suppress the warning; - certain uses of package, protected, or public visibility of fields, which would have required a fairly large number of changes to substitute calls to new getX() or setX(...) methods; having went through the process, there is now a precise record of when a suppression was required, so this will allow us (over time) to further fine tune the check style rules and remove these suppressions or bless them as exceptions; but unlike Vincent, I believe there is no value in delaying applying the patch until such fine tuning occurs; i view that as an unnecessary, and counterproductive delay; it is better to patch first, then fine tune over time; when we do fine tune, I would suggest the following to deal with the above suppressions: - refactor code that breaks the parameter count, method length maximum; - maybe refactor code that breaks the file length maximum, but i'm not sure I'm ready to advocate that; - expand the regular expression that checks static final fields to explicitly allow "log"; even then, there are some other cases where it is desirable to not force a private static final to be all upper case, particularly in those cases where it is typed as an object reference (as opposed to a string or a literal constant); - consider changing most or all package/protected/public non constant field members to private, introducing use of getter/setter methods; but there may remain exceptions for efficiency reasons, e.g., to avoid method calls in very frequently accessed fields; though this could be reduced by declaring such methods to be final, allowing compilers to inline the access; i'm not sure if I would advise a change on inner assignments; sometimes it is useful to find potential programming errors; on the other hand, i happen to like using inner assignments, as it makes the code flow more smoothly for me and looks better on the screen; although i admit a long history of coding C which goes back to the 70s, wherein this is a common usage pattern; i guess i would prefer leaving the existing check, and then having explicit use of CSOK when an author has made a conscious judgement call that it is not a programming error; but i repeat again, let's not try to work out these tweaks to the checkstyle rules now; let's start by cleaning the whole lot of warnings out, and then use piece-wise iteration to improve the situation over time; I am not happy to remove that file, although I must admit that my reasons > are selfish. I have an older IDE that can't integrate with checkstyle 5 and > I'm not ready to pay for a new license just for that reason. Is the > checkstyle-4.0.xml file causing any problem? I don't see why it must be > deleted. > that is a reasonable objection to removing the file, so indeed i would not object to leaving it in place; however, i did not test checkstyle 1.4, and indeed, there are a few changes that appear a
Re: [Bug 49733] [PATCH] resolve compilation, checkstyle, javadoc warnings
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 wrote: > Inline below. > > On Wed, Aug 11, 2010 at 7:45 PM, Vincent Hennebert > 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
Re: [Bug 49733] [PATCH] resolve compilation, checkstyle, javadoc warnings
Inline below. On Wed, Aug 11, 2010 at 7:45 PM, Vincent Hennebert 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 u
Re: [Bug 49733] [PATCH] resolve compilation, checkstyle, javadoc warnings
Vincent Hennebert wrote: Hi All, First of all let me start by saying many thanks to Glenn for the hours and hours of effort that he must of put into going through the code and resolving the checkstyle warnings. Thanks Glenn! 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. I personally don't have problems with the checkstyle rules and whilst I know there have been some unhappiness expressed in the past with the current rules set no one has put forward any improvements. Also I think it may be impossible to find a perfect set of checkstyle rules. In some cases the tool just isn't flexible enough in other cases it's impossible to reach consenus, so I tend to agree with Glenn here. Let's apply the parts of the patch we agree with now instead of waiting for a new set of checkstyle rules which may never be developed. 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. I agree with Vincent here. I don't like the code being cluttered with CSOFF comments and it does defeat the purpose of checkstyle. Why not just accept that there will still be a few warnings left after the patch is applied. The patch is still a huge improvement on the current situation and we can try to address the remaining few warnings over time. 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 am not happy to remove that file, although I must admit that my reasons are selfish. I have an older IDE that can't integrate with checkstyle 5 and I'm not ready to pay for a new license just for that reason. Is the checkstyle-4.0.xml file causing any problem? I don't see why it must be deleted. 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. Thanks, Chris 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, ha
Re: [Bug 49733] [PATCH] resolve compilation, checkstyle, javadoc warnings
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
RE: [Bug 49733] [PATCH] resolve compilation, checkstyle, javadoc warnings
While it may be possible in Java as it was in old languages like C to write an entire program on one line, it is more readable to break it out. I agree there shouldn't be an arbitrary limit such as no line longer than 100 or 80 or 200 characters. Sometimes it may make sense to string more than that together. It should be at the discretion of the programmer to limit line lengths for readability, to make it easier for debugging and modifying later. Unreasonably long lines should be broken down where possible. If the line gets long because you have a parameter to a method which is the result of another method, you could create an object of that parameter type using that method on the line above and pass the object instead to shorten it. It may take an extra nanosecond to execute, or add a tiny bit of disk or memory space use, but if we were really that concerned we'd still be writing code using A and B as variable names.. From: Glenn Adams [mailto:gl...@skynav.com] Sent: Tuesday, August 10, 2010 10:48 AM To: fop-dev@xmlgraphics.apache.org Subject: Re: [Bug 49733] [PATCH] resolve compilation, checkstyle, javadoc warnings On Tue, Aug 10, 2010 at 9:09 PM, Jeremias Maerki wrote: fine-tuning CheckStyle is probably a good thing. oh, i completely agree on that, and i'm sure we will find that opportunity as time permits, and when it does, i will happily contribute a number of comments for consideration; but that conversation is precisely what i wish to avoid at this juncture, since it will undoubtedly prove to be quite subjective; for example, how long should a line of code be permitted to be? the currently written FOP checkstyle rule is 100 characters, but why not 80 or 132? my own coding style is to use a single line for an expression regardless its length: the fact that my editor (emacs) happens to wrap at 200 characters is merely a property of my favorite font, screen resolution, window size, and wrap configuration; did I follow the rule in my new complex script code? no, i used my favorite style and disabled this warning; but i didn't make the decision randomly, and i didn't want to leave a visible warning during style checking; the CS keywords remain in the code, and can be used to evaluate or audit how far real practice diverges from an artificially prescribed practice (the encoded rules); what is my reasoning? artificial line breaks make code harder to read and understand, or at least that is my considered conclusion having read and written much code (though I would make an exception for APL - short lines are definitely better there); on the other hand, while fixing the current code, there were occasions where reported style violations marked possible coding errors; for example, the lack of a default clause in a switch statement was something i encountered a few dozen times in this cleanup work; in some of the cases, it was not a semantic error, but in others it was a semantic error waiting for a bug report; although I did not address findbugs errors in this pass, there are a few errors that it detects that can easily lead to semantic errors, such as copying or returning references to live arrays (as opposed to performing deep copies); in some cases, it is desirable, for performance reasons, to return a reference to a live array typed member of an object; however, in other cases, it is an invitation to unintentional side effects leading to dynamic errors; in these cases, a contributor or committer has to make a decision based on a deeper knowledge of code function and usage, and not merely upon the surface form; i saved the task of fixing findbugs errors (and PMD errors) until the low hanging fruit has been picked, namely the subject of the current patch; having plucked the easy ones, i'll continue to the more tricky ones, but that will have to wait until the current patch is in the trunk and no regressions appear; g.
Re: [Bug 49733] [PATCH] resolve compilation, checkstyle, javadoc warnings
On Tue, Aug 10, 2010 at 9:09 PM, Jeremias Maerki wrote: > fine-tuning CheckStyle is probably a good thing. oh, i completely agree on that, and i'm sure we will find that opportunity as time permits, and when it does, i will happily contribute a number of comments for consideration; but that conversation is precisely what i wish to avoid at this juncture, since it will undoubtedly prove to be quite subjective; for example, how long should a line of code be permitted to be? the currently written FOP checkstyle rule is 100 characters, but why not 80 or 132? my own coding style is to use a single line for an expression regardless its length: the fact that my editor (emacs) happens to wrap at 200 characters is merely a property of my favorite font, screen resolution, window size, and wrap configuration; did I follow the rule in my new complex script code? no, i used my favorite style and disabled this warning; but i didn't make the decision randomly, and i didn't want to leave a visible warning during style checking; the CS keywords remain in the code, and can be used to evaluate or audit how far real practice diverges from an artificially prescribed practice (the encoded rules); what is my reasoning? artificial line breaks make code harder to read and understand, or at least that is my considered conclusion having read and written much code (though I would make an exception for APL - short lines are definitely better there); on the other hand, while fixing the current code, there were occasions where reported style violations marked possible coding errors; for example, the lack of a default clause in a switch statement was something i encountered a few dozen times in this cleanup work; in some of the cases, it was not a semantic error, but in others it was a semantic error waiting for a bug report; although I did not address findbugs errors in this pass, there are a few errors that it detects that can easily lead to semantic errors, such as copying or returning references to live arrays (as opposed to performing deep copies); in some cases, it is desirable, for performance reasons, to return a reference to a live array typed member of an object; however, in other cases, it is an invitation to unintentional side effects leading to dynamic errors; in these cases, a contributor or committer has to make a decision based on a deeper knowledge of code function and usage, and not merely upon the surface form; i saved the task of fixing findbugs errors (and PMD errors) until the low hanging fruit has been picked, namely the subject of the current patch; having plucked the easy ones, i'll continue to the more tricky ones, but that will have to wait until the current patch is in the trunk and no regressions appear; g.
RE: [Bug 49733] [PATCH] resolve compilation, checkstyle, javadoc warnings
I believe English has more words than any other language, which is why they say it's the easiest language to learn and the hardest language to master. I would think warnings like calls to deprecated methods should be fixed asap. Other warnings like dead code and unused variables I would guess were left in for future reference, such as methods planned to be added later or methods removed leaving code in place in case they need added back in later. These should be removed if there's another way to document notes for potential future enhancements. If they were just sloppy coding, putting in or leaving in stuff that has no reason to be, I agree with Glenn it should be cleaned out. While high quality might refer more to making a program that does what the users need and doesn't crash, I'd agree coding should still need some "standard of quality" to include error free code. It could discourage new developers from joining the project if they get a copy of the Trunk and see all those warnings even if it does run despite them. -Original Message- From: Jeremias Maerki [mailto:d...@jeremias-maerki.ch] Sent: Tuesday, August 10, 2010 9:09 AM To: fop-dev@xmlgraphics.apache.org Subject: Re: [Bug 49733] [PATCH] resolve compilation, checkstyle, javadoc warnings Well, I'm not a native English-speaker, so maybe my choice of the word "rant" was too much. Anyway, Glenn, we're not too far apart. I try to remove warnings whenever I change a class, i.e. gradual improvement as time allows. Sometimes CheckStyle would bark at something that I didn't consider a problem so I ignored it. That's why I mentioned that fine-tuning CheckStyle is probably a good thing. From time to time, people would fix a bunch of classes, but a thorough attempt such as yours hasn't happened, yet. So this is a chance for us and your work is definitely appreciated. I don't think we've had any voice, yet, who said that fixing the issues was a bad idea. On 10.08.2010 14:46:28 Glenn Adams wrote: > my apologies if my statement appeared to be a "rant", as it was not > intended as such; > > perhaps my emphasis was an exaggeration, but if one goes through the > trouble to add build rules for style checking, bug finding, and code > quality reporting, then it does appear odd to ignore them, which was > my reaction to the current code base and vincent's response; > > i admit that i prefer a zero warning policy, and i have attempted at > every opportunity to introduce or enforce such policy on the many dev > projects I've managed or participated in over four decades; in > general, i find it helps me and other devs, particularly as a way of > finding new noise we are introducing; if there is already a lot of > noise in the system, it is easy to ignore new noise, which is > precisely what i would like to avoid in my own > contributes: contributing to the noise level; > > note that i am not arguing for or against a specific set of policy > rules, just that whatever they are, they get implemented and enforced, > while knowing at the same time that every rule has exceptions, and > that mechanisms to provide filtering adequately address this point; > furthermore, arguing over which a particular exception is justified or > not can become a great waste of time; as I've stated previously, if a > contributor or committer has made the conscious choice to disable a > warning, then I'm happy to accept their judgement, as long as it is > done in a thoughtful way, and not merely as a way of ignoring rules; > > if the majority of committers feel it best not to patch these warnings > and move on from there, then i'll readily submit to that consensus; my > hope, however, is that this contribution can positively contribute to > FOP and the community, so it is natural that I would prefer it not be > delayed for some unknown process to create a "new consensus" on style > rules; > > regards, > glenn > > On Tue, Aug 10, 2010 at 8:07 PM, 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
Re: [Bug 49733] [PATCH] resolve compilation, checkstyle, javadoc warnings
Well, I'm not a native English-speaker, so maybe my choice of the word "rant" was too much. Anyway, Glenn, we're not too far apart. I try to remove warnings whenever I change a class, i.e. gradual improvement as time allows. Sometimes CheckStyle would bark at something that I didn't consider a problem so I ignored it. That's why I mentioned that fine-tuning CheckStyle is probably a good thing. From time to time, people would fix a bunch of classes, but a thorough attempt such as yours hasn't happened, yet. So this is a chance for us and your work is definitely appreciated. I don't think we've had any voice, yet, who said that fixing the issues was a bad idea. On 10.08.2010 14:46:28 Glenn Adams wrote: > my apologies if my statement appeared to be a "rant", as it was not intended > as such; > > perhaps my emphasis was an exaggeration, but if one goes through the trouble > to add build rules for style checking, bug finding, and code quality > reporting, then it does appear odd to ignore them, which was my reaction to > the current code base and vincent's response; > > i admit that i prefer a zero warning policy, and i have attempted at every > opportunity to introduce or enforce such policy on the many dev projects > I've managed or participated in over four decades; in general, i find it > helps me and other devs, particularly as a way of finding new noise we are > introducing; if there is already a lot of noise in the system, it is easy to > ignore new noise, which is precisely what i would like to avoid in my own > contributes: contributing to the noise level; > > note that i am not arguing for or against a specific set of policy rules, > just that whatever they are, they get implemented and enforced, while > knowing at the same time that every rule has exceptions, and that mechanisms > to provide filtering adequately address this point; furthermore, arguing > over which a particular exception is justified or not can become a great > waste of time; as I've stated previously, if a contributor or committer has > made the conscious choice to disable a warning, then I'm happy to accept > their judgement, as long as it is done in a thoughtful way, and not merely > as a way of ignoring rules; > > if the majority of committers feel it best not to patch these warnings and > move on from there, then i'll readily submit to that consensus; my hope, > however, is that this contribution can positively contribute to FOP and the > community, so it is natural that I would prefer it not be delayed for some > unknown process to create a "new consensus" on style rules; > > regards, > glenn > > On Tue, Aug 10, 2010 at 8:07 PM, 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 witho
Re: [Bug 49733] [PATCH] resolve compilation, checkstyle, javadoc warnings
my apologies if my statement appeared to be a "rant", as it was not intended as such; perhaps my emphasis was an exaggeration, but if one goes through the trouble to add build rules for style checking, bug finding, and code quality reporting, then it does appear odd to ignore them, which was my reaction to the current code base and vincent's response; i admit that i prefer a zero warning policy, and i have attempted at every opportunity to introduce or enforce such policy on the many dev projects I've managed or participated in over four decades; in general, i find it helps me and other devs, particularly as a way of finding new noise we are introducing; if there is already a lot of noise in the system, it is easy to ignore new noise, which is precisely what i would like to avoid in my own contributes: contributing to the noise level; note that i am not arguing for or against a specific set of policy rules, just that whatever they are, they get implemented and enforced, while knowing at the same time that every rule has exceptions, and that mechanisms to provide filtering adequately address this point; furthermore, arguing over which a particular exception is justified or not can become a great waste of time; as I've stated previously, if a contributor or committer has made the conscious choice to disable a warning, then I'm happy to accept their judgement, as long as it is done in a thoughtful way, and not merely as a way of ignoring rules; if the majority of committers feel it best not to patch these warnings and move on from there, then i'll readily submit to that consensus; my hope, however, is that this contribution can positively contribute to FOP and the community, so it is natural that I would prefer it not be delayed for some unknown process to create a "new consensus" on style rules; regards, glenn On Tue, Aug 10, 2010 at 8:07 PM, 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? > > > >
Re: [Bug 49733] [PATCH] resolve compilation, checkstyle, javadoc warnings
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, wrote: > > > https://issues.apache.org/bugzilla/show_bug.cgi?id=49733 > > > > --- Comment #5 from Vincent Hennebert 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
Re: [Bug 49733] [PATCH] resolve compilation, checkstyle, javadoc warnings
a.k.a. CTR (commit then review) On Tue, Aug 10, 2010 at 1:54 PM, Glenn Adams wrote: > Regarding the patch I just posted, I would recommend applying CBR policy to > get it into the trunk ASAP before other changes are committed. Of course, > the present committers will decide the appropriate process yourselves, but > that is my input. > > Regards, > Glenn > >
