inline
On Thu, Aug 12, 2010 at 8:25 PM, Jeremias Maerki <d...@jeremias-maerki.ch>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.
