xmlgraphics-general mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Glenn Adams <gl...@skynav.com>
Subject Re: Checkstyle, Reloaded
Date Fri, 03 Feb 2012 21:20:25 GMT
which version of checkstyle are you using? there are two errors in parsing
the proposed checkstyle file with 5.1;

   <!-- <property name="ignoreEnhancedForColon" value="false"/> -->
   <!-- <module name="OneStatementPerLine"/> -->


once i fixed the checkstyle file to work with 5.1, i see that 4935 and
31935 new warnings/errors are introduced in trunk and in my i18n branches,
respectively; clearly, this is going to require a large amount of editing
to allow use of the proposed rules;

many of the new errors I notice (in both trunk and my i18n branches) have
to do with whitespace before or after '(', ')', and cast operations; i do
not agree with enforcing the presence or absence of whitespace around these
constructs; i happen to always use whitespace before and after parens,
e.g., the following should produce no checkstyle warning:

public int foo ( int a, int b, int c ) {
  return bar ( a, b, c );
};

i would like whitespace after '{' and before '}' in an array
initialization, e.g., both of the following should be permitted:

int[] a = new int[] { 1, 2, 3 };
int[] a = new int[] {1, 2, 3};

i would like SimplifyBooleanReturn to be removed;

i would like whitespace after BNOT produce a warning, e.g. both ! foo and
!foo should be accepted without warning;

i would like whitespace after DOT operator to be permissible, e.g., both
x.y and x . y should be permitted;

i would like empty blocks to be permissible, e.g., the following should be
permitted:

if ( test ) {
  /* TBD - handle test is true */
} else {
  /* TBD - handle test is false */
}

i would like the arbitrary line length rule to be removed; i do not agree
to 110 line length; or if you insist, i could accept 150;

i do not agree with including MultipleVariableDeclarations rule; i
routinely define multiple local variables in one statement, e.g., int x, y;

i do not agree with requiring LocalFinalVariableName to match
'^[A-Z][A-Z0-9]*(_[A-Z0-9]+)*$';
instead, it should continue to match the currently used
pattern "^[a-z][a-zA-Z0-9]*$";

why are there two NoWhitespaceAfter rules?

    <module name="NoWhitespaceAfter">
      <property name="tokens" value="ARRAY_INIT"/>
    </module>
   <module name="NoWhitespaceAfter">
      <property name="allowLineBreaks" value="false"/>
      <property name="tokens"
value="BNOT,DEC,DOT,INC,LNOT,UNARY_MINUS,UNARY_PLUS"/>
    </module>

if you fix the above problems, then i will re-run on trunk and my i18n
branch to check if there are any other issues that need to be resolved;

On Fri, Feb 3, 2012 at 10:45 AM, Vincent Hennebert <vhennebert@gmail.com>wrote:

> Hi All,
>
> it is well-known that people are not happy with the Checkstyle file we
> have in FOP. And there’s no point enforcing the application of
> Checkstyle rules if we don’t agree with them in the first place.
>
> I’ve finally taken on me to create a new Checkstyle file that follows
> modern development practices. I’ve been testing it on my own projects
> for a few months now and I’m happy with it, so I’d like to share it with
> the community. The idea is that once we’ve reached consensus on the
> Checkstyle rules we want to apply, we could set up a no warning policy
> and enforce it by running Checkstyle in CI.
>
> I’m also taking this as an opportunity to propose that we adopt a common
> Checkstyle policy to all the sub-projects of XML Graphics. So once we’ve
> agreed on a set of rules we would apply them to FOP and XGC immediately,
> and eventually also to Batik, and keep them in sync.
>
> We would also apply the rules to the test files as well as the main
> code. Tests are as important as the actual code and there is no reason
> why they shouldn’t be checked.
>
> It is likely that the current code will not be compliant with the new
> rules. However, most of them are really just about the syntax, so
> I believe it should be fairly straightforward to make the code at least
> 90% compliant just by applying Eclipse’s command-line code formatter.
>
> Please find the Checkstyle file attached. It is based on Checkstyle 5.5
> and basically follows Sun’s recommendations for Java styling with a few
> adaptations. What’s noteworthy is the following:
>
> • Removed checks for Javadoc. What we want is quality Javadoc, and that
>  is not something that Checkstyle can check. Having Javadoc checks is
>  counter-productive as it forces us to put {@inheritDoc} everywhere, or
>  to create truly useless doc like the following:
>  /**
>   * Returns the thing.
>   * @return the thing
>   */
>  public Thing getThing() {
>      return thing;
>  }
>  This is just clutter really. I think it should be left to peer review
>  to check whether a Javadoc comment is properly written, or whether the
>  lack thereof is justified. There’s an excellent blog entry from
>  Torsten Curdt about this:
>  http://vafer.org/blog/20050323095453/
> • Removed check for file and method lengths. I don’t think it makes
>  sense to define a maximum size for files and methods. Sometimes
>  a 10-line method is way too big, sometimes it makes sense to have it
>  reach 20 lines. Same for files: it’s ok to reach 1000 lines if the
>  class contains several inner classes. If it doesn’t, then it’s
>  probably too big. I don’t think there is any definite figure we can
>  agree on and blindly follow, so I think sizes should be left to peer
>  review.
> • However, I left the check for maximum line length because unreasonably
>  long lines make the code hard to follow. I increased it to 110
>  though to follow the evolution of monitor sizes. But as Peter
>  suggested me, we probably want to keep it low in order to make
>  side-by-side comparison easy.
> • I added a check for the order of imports; this is to reduce noise in
>  diffs when committing. I think most of us have configured their IDE to
>  automatically organise imports when saving changes to a file. This is
>  a great feature because it allows to keep the list of imports
>  up-to-date. But in order to avoid constant back and forth changes when
>  different committers change the same file, I think it makes sense that
>  we all have the same configuration. I modeled this list after
>  Jeremias’ one, that I progressively inferred from his commits.
>
> Please let me know what you think. I’m inclined to follow lazy consensus
> on this, and apply the proposed changes if nobody has objected within
> 2 weeks. If anybody feels that a formal vote is necessary, feel free to
> say so.
>
> Thanks,
> Vincent
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: general-unsubscribe@xmlgraphics.apache.org
> For additional commands, e-mail: general-help@xmlgraphics.apache.org
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message