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

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.


Reply via email to