@ Chris
On Wed, Aug 11, 2010 at 11:20 PM, Chris Bowditch <bowditch_ch...@hotmail.com
> 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 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:
inside TreeWalker module, add:
<module name="FileContentsHolder"/>
inside Checker module, add:
<module name="SuppressionFilter">
<property name="file" value="${samedir}/checkstyle-suppressions.xml"/>
</module>
<module name="SuppressionCommentFilter">
<property name="offCommentFormat" value="CSOFF\: ([\w\|]+)"/>
<property name="onCommentFormat" value="CSON\: ([\w\|]+)"/>
<property name="checkFormat" value="$1"/>
</module>
<module name="SuppressWithNearbyCommentFilter">
<property name="commentFormat" value="CSOK\: ([\w\|]+)"/>
<property name="checkFormat" value="$1"/>
<property name="influenceFormat" value="0"/>
</module>
G.
