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 Mon, 06 Feb 2012 17:58:43 GMT
overall, the i believe the issue of whitespace usage is a matter of
personal style, and should not be subject to strict rule enforcement; as
long as i can use CSOFF to disable rules on source files i create, then i
can accept rules which encode different usage patterns;

i believe it is more important to preserve the style of the original author
of a given source file rather than attempt to follow an arbitrary usage
pattern in this regard; i don't mind using rules that differ from mine when
those source files were authored by those different usage patterns; but i
do not agree with enforcing them in my own style for a variety of reasons:

   - it slows me down
   - it makes it harder for me to read my own code, since i am accustomed
   to reading my style

ideally, i believe you should craft rules that are sufficiently flexible in
the area of personal style choices that accommodate all of our preferences;
however, if it is acceptable to deal with exceptions using CSOFF, etc.,
then that would be sufficient for me

On Mon, Feb 6, 2012 at 10:09 AM, Vincent Hennebert <vhennebert@gmail.com>wrote:

> Hi Glenn,
>
> Thanks for taking the time to look at this. Looks like we should be able
> to reach a consensus without too much difficulty.
>
> On 03/02/12 21:20, Glenn Adams wrote:
> > 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;
>
> Like I said most of them are purely about syntax and are easily solved
> with a code formatting tool. Obviously I’m happy to run such a tool on
> your own Git branches and submit a patch if that can help you.
>

i prefer not to use automatic tools to make fixups in this case for the
reasons that chris outlined


> > 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’d rather keep the rule, as it enforces standard Java style that will
> be easily recognized by any Java developer. I also find the variation in
> the use of whitespace to be one of the most distracting things when
> reading code.
>
> That said, if that really bothers you I would be ok with relaxing the
> rule, except for the whitespace between a method call and the left
> parenthesis, to make it clear that it’s a method call.
>

i prefer my style of using whitespace;

if you are not insisting that no CSOFF declarations appear in source code,
then I can accept your proposal, provided I can use CSOFF to disable this
rule for source files that i create (for those i didn't create, i can
adhere to the rule)


> > 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};
>
> Yep, no problem.
>
>
> > i would like SimplifyBooleanReturn to be removed;
>
> Hmmm. Ok.
>
>
> > i would like whitespace after BNOT produce a warning, e.g. both ! foo and
> > !foo should be accepted without warning;
>
> I’d keep the rule. Allowing a standalone exclamation point is too
> dangerous I think. Too easy to miss.
>

again, if you don't mind me using CSOFF in source files I author, then I
can accept


>
>
> > i would like whitespace after DOT operator to be permissible, e.g., both
> > x.y and x . y should be permitted;
>
> Why? Note that it’s possible to break the line before the dot.


when i cast an object reference then invoke a method, i like to use the
following whitespace:

( (Foo) obj ) . doit ( ... )

again, if you don't mind me using CSOFF in source files I author, then I
can accept


>
> > 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 find that it’s just clutter, but I don’t really mind.
>

the issue is i would like to put comments into blocks that are otherwise
empty; this is useful as a reminder to me as a coder that i may need to do
something for those blocks in the future that make them non-empty

>
>
> > 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’m afraid I’m not happy to go any higher than 110. Pascal actually made
> a good point by saying that there is only a certain number of tokens
> that a human being can grasp on one line.
>
> Also, having to scroll horizontally is a real pain and greatly impedes
> the readability of code. And if the editor is set up to automatically
> wrap long lines then this ruins the indentation, which is not any
> better.
>

my screen width and editor (emacs) and font size allows me to use 150
characters without scrolling; i think this is just as reasonable as 110;

frankly both of these are arbitrary; rather than spending time arguing
about arbitrary limits, i would prefer simply removing this rule/limit, and
leaving it to peer review


>
>
> > i do not agree with including MultipleVariableDeclarations rule; i
> > routinely define multiple local variables in one statement, e.g., int x,
> y;
>
> I’d really rather keep the rule. Variables should be used (i.e.,
> initialized) as soon as they are declared anyway. Otherwise there is
> a risk that with time, the variable declaration appears further and
> further away from where it is used.
>

i prefer being able to use multiple declarations in one statement when it
makes sense; the language allows it, so why should we impose an arbitrary
rule to prevent it?

again, if you don't mind me using CSOFF in source files I author, then I
can accept


>
>
> > 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]*$";
>
> Unless I missed something this is the default in Checkstyle 5.5.
>

i got a warning under Checkstyle 5.1; so i guess the default has changed;
in order to avoid being subject to this change, i suggest it be made
explicit


>
>
> > 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>
>
> Because one rule allows a line break after the token, the other not. But
> anyway, following your comment we would remove the former one.
>
>
ok


>
> > 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;
>
> Let me know what you think. Hopefully others will also speak up to share
> their views.
>
> Thanks,
> Vincent
>
>
> > 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
> >>
> >
>
> ---------------------------------------------------------------------
> 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