Mailing-List: contact general-help@xmlgraphics.apache.org; run by ezmlm
Precedence: bulk
Reply-To: general@xmlgraphics.apache.org
Received-SPF: neutral (nike.apache.org: local policy)
MIME-Version: 1.0
In-Reply-To: <4F300955.6030006@gmail.com>
References: <4F2C1D25.8010501@gmail.com>
 <CACQ=j+fuosd_5W09LDnNeCbo-rN+2kpsdqnbH752iH1-N+HJdQ@mail.gmail.com>
 <4F300955.6030006@gmail.com>
From: Glenn Adams <glenn@skynav.com>
Date: Mon, 6 Feb 2012 10:58:43 -0700
Message-ID: 
 <CACQ=j+cEunN8_d0O=dUPCHmMsK9+71Pj3f4VYk23xZMrxuMuuA@mail.gmail.com>
Subject: Re: Checkstyle, Reloaded
To: general@xmlgraphics.apache.org
Content-Type: multipart/alternative; boundary=e89a8ff256b0f4c7f204b84f6a5d

--e89a8ff256b0f4c7f204b84f6a5d
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

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>wr=
ote:

> 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=3D"ignoreEnhancedForColon" value=3D"false"/> -->
> >    <!-- <module name=3D"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 editi=
ng
> > 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=E2=80=99m happy to run such a to=
ol 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) ha=
ve
> > 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=E2=80=99d 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=E2=80=99s 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 =3D new int[] { 1, 2, 3 };
> > int[] a =3D 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 a=
nd
> > !foo should be accepted without warning;
>
> I=E2=80=99d 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., bot=
h
> > x.y and x . y should be permitted;
>
> Why? Note that it=E2=80=99s 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=E2=80=99s just clutter, but I don=E2=80=99t 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 agr=
ee
> > to 110 line length; or if you insist, i could accept 150;
>
> I=E2=80=99m afraid I=E2=80=99m not happy to go any higher than 110. Pasca=
l 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=E2=80=99d 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=3D"NoWhitespaceAfter">
> >       <property name=3D"tokens" value=3D"ARRAY_INIT"/>
> >     </module>
> >    <module name=3D"NoWhitespaceAfter">
> >       <property name=3D"allowLineBreaks" value=3D"false"/>
> >       <property name=3D"tokens"
> > value=3D"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.co=
m
> >wrote:
> >
> >> Hi All,
> >>
> >> it is well-known that people are not happy with the Checkstyle file we
> >> have in FOP. And there=E2=80=99s no point enforcing the application of
> >> Checkstyle rules if we don=E2=80=99t agree with them in the first plac=
e.
> >>
> >> I=E2=80=99ve finally taken on me to create a new Checkstyle file that =
follows
> >> modern development practices. I=E2=80=99ve been testing it on my own p=
rojects
> >> for a few months now and I=E2=80=99m happy with it, so I=E2=80=99d lik=
e to share it with
> >> the community. The idea is that once we=E2=80=99ve reached consensus o=
n the
> >> Checkstyle rules we want to apply, we could set up a no warning policy
> >> and enforce it by running Checkstyle in CI.
> >>
> >> I=E2=80=99m also taking this as an opportunity to propose that we adop=
t a common
> >> Checkstyle policy to all the sub-projects of XML Graphics. So once we=
=E2=80=99ve
> >> agreed on a set of rules we would apply them to FOP and XGC immediatel=
y,
> >> 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=E2=80=99t 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 leas=
t
> >> 90% compliant just by applying Eclipse=E2=80=99s command-line code for=
matter.
> >>
> >> Please find the Checkstyle file attached. It is based on Checkstyle 5.=
5
> >> and basically follows Sun=E2=80=99s recommendations for Java styling w=
ith a few
> >> adaptations. What=E2=80=99s noteworthy is the following:
> >>
> >> =E2=80=A2 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, o=
r
> >>  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 th=
e
> >>  lack thereof is justified. There=E2=80=99s an excellent blog entry fr=
om
> >>  Torsten Curdt about this:
> >>  http://vafer.org/blog/20050323095453/
> >> =E2=80=A2 Removed check for file and method lengths. I don=E2=80=99t t=
hink 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=E2=80=99s ok to reach 1000 lines i=
f the
> >>  class contains several inner classes. If it doesn=E2=80=99t, then it=
=E2=80=99s
> >>  probably too big. I don=E2=80=99t think there is any definite figure =
we can
> >>  agree on and blindly follow, so I think sizes should be left to peer
> >>  review.
> >> =E2=80=A2 However, I left the check for maximum line length because un=
reasonably
> >>  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.
> >> =E2=80=A2 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 t=
o
> >>  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 whe=
n
> >>  different committers change the same file, I think it makes sense tha=
t
> >>  we all have the same configuration. I modeled this list after
> >>  Jeremias=E2=80=99 one, that I progressively inferred from his commits=
.
> >>
> >> Please let me know what you think. I=E2=80=99m 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 t=
o
> >> 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
>
>

--e89a8ff256b0f4c7f204b84f6a5d--