On Fri, Aug 13, 2010 at 4:32 PM, Simon Pepping <spepp...@leverkruid.eu>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.
