You are viewing a plain text version of this content. The canonical link for it is here.
Posted to general@xmlgraphics.apache.org by Vincent Hennebert <vh...@gmail.com> on 2012/02/03 18:45:09 UTC
Checkstyle, Reloaded
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
Re: Checkstyle, Reloaded
Posted by Chris Bowditch <bo...@hotmail.com>.
On 03/02/2012 21:20, Glenn Adams wrote:
Hi Glen,
> which version of checkstyle are you using? there are two errors in parsing
> the proposed checkstyle file with 5.1;
Vincent says checkstyle v5.5 was used in his original e-mail.
>
> <!--<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;
I agree that seems way too many new warnings/errors.
Chris
>
> 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<vh...@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
Re: Checkstyle, Reloaded
Posted by Glenn Adams <gl...@skynav.com>.
On Fri, Feb 3, 2012 at 2:20 PM, Glenn Adams <gl...@skynav.com> wrote:
> 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;
>
actually, i generally use whitespace after a cast, but i notice much
existing fop code does not; so i think both styles should be permitted;
> i would like whitespace after BNOT produce a warning, e.g. both ! foo and
> !foo should be accepted without warning;
>
s/produce a warning/not produce a warning/
Re: Checkstyle, Reloaded
Posted by Vincent Hennebert <vh...@gmail.com>.
On 07/02/12 13:52, Glenn Adams wrote:
> in general, i object to rules that attempt to prescribe whitespace usage
> and line length restrictions; i do not mind rules that enforce naming
> conventions, indentation rules, tab versus space usage, newline usage, and
> a variety of other styles
>
> the use of checkstyle should not be an unnecessary burden on this
> community, or on our productivity
>
> the rules on consensus in this community appear to be that one negative
> vote is sufficient to prevent some action,
Well, in the present case this is merely an informal discussion about
what we would like to have or not have in our Checkstyle file. If we
feel that a vote is necessary then it’ll come later and Apache rules
will apply:
http://apache.org/foundation/voting.html
> so my vote would be negative if
> asked about enforcing the following (in priority order):
>
> - line length constraints
> - white space around parenthesis, braces, brackets
> - multiple variables declarations per statement
>
> regarding use of CSOFF, i do not agree that it causes clutter, any more
> than the use of assert does
<snip/>
Vincent
---------------------------------------------------------------------
To unsubscribe, e-mail: general-unsubscribe@xmlgraphics.apache.org
For additional commands, e-mail: general-help@xmlgraphics.apache.org
Re: Checkstyle, Reloaded
Posted by Glenn Adams <gl...@skynav.com>.
in general, i object to rules that attempt to prescribe whitespace usage
and line length restrictions; i do not mind rules that enforce naming
conventions, indentation rules, tab versus space usage, newline usage, and
a variety of other styles
the use of checkstyle should not be an unnecessary burden on this
community, or on our productivity
the rules on consensus in this community appear to be that one negative
vote is sufficient to prevent some action, so my vote would be negative if
asked about enforcing the following (in priority order):
- line length constraints
- white space around parenthesis, braces, brackets
- multiple variables declarations per statement
regarding use of CSOFF, i do not agree that it causes clutter, any more
than the use of assert does
On Tue, Feb 7, 2012 at 1:57 AM, Chris Bowditch
<bo...@hotmail.com>wrote:
> On 06/02/2012 22:57, Alexios Giotis wrote:
>
>> Hi,
>>
>
> Hi Alexios,
>
>
>> I can't see a point having checkstyle rules and then adding CSOFF on new
>> files to disable them. It is faster to read, debug or fix source code when
>> there is uniformity rather than every file having the personal style of
>> the initial author. It would be helpful to additionally have configurations
>> for popular Java IDEs so that developers write code, press the format
>> keyboard shortcut and know that the output is acceptable for a patch.
>> Eclipse calls them code formatter profiles and they can be exported and
>> imported.
>>
>
> You raise a very good point and I agree that CSOFF isn't a practical
> option. Having CSOFF everywhere in the code adds clutter and makes it
> harder to read. We need to try and reach consenus on the checkstyle rules,
> by abandoning some but not too many of the rules. Glenn, would you be able
> to list the rules you object with in an order of priority, with the ones
> that would inconvience you the most at the top of the list with the least
> annoying ones at the bottom? I think that would help us arrive at some sort
> of compromise.
>
>
>> Related to line length, I would go for a maximum of 100. As already said,
>> there is a limit on the amount of information that can be easily understood
>> per line. More than this typically indicates methods with too many
>> arguments or deep nesting that should be refactored into methods. Also, I
>> really hate working with my laptop or going with it to a customer site for
>> support and having to horizontally scroll in file diffs.
>>
>
> Thanks,
>
> Chris
>
>
>
>> Alexios Giotis
>>
>>
>>
>> On Feb 6, 2012, at 7:58 PM, Glenn Adams wrote:
>>
>> 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/<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<ge...@xmlgraphics.apache.org>
>>>>>> For additional commands, e-mail: general-help@xmlgraphics.**
>>>>>> apache.org <ge...@xmlgraphics.apache.org>
>>>>>>
>>>>>> ------------------------------**------------------------------**
>>>> ---------
>>>> To unsubscribe, e-mail: general-unsubscribe@**xmlgraphics.apache.org<ge...@xmlgraphics.apache.org>
>>>> For additional commands, e-mail: general-help@xmlgraphics.**apache.org<ge...@xmlgraphics.apache.org>
>>>>
>>>>
>>>>
>> ------------------------------**------------------------------**---------
>> To unsubscribe, e-mail: general-unsubscribe@**xmlgraphics.apache.org<ge...@xmlgraphics.apache.org>
>> For additional commands, e-mail: general-help@xmlgraphics.**apache.org<ge...@xmlgraphics.apache.org>
>>
>>
>>
>>
>
> ------------------------------**------------------------------**---------
> To unsubscribe, e-mail: general-unsubscribe@**xmlgraphics.apache.org<ge...@xmlgraphics.apache.org>
> For additional commands, e-mail: general-help@xmlgraphics.**apache.org<ge...@xmlgraphics.apache.org>
>
>
Re: Checkstyle, Reloaded
Posted by Chris Bowditch <bo...@hotmail.com>.
On 06/02/2012 22:57, Alexios Giotis wrote:
> Hi,
Hi Alexios,
>
> I can't see a point having checkstyle rules and then adding CSOFF on new files to disable them. It is faster to read, debug or fix source code when there is uniformity rather than every file having the personal style of the initial author. It would be helpful to additionally have configurations for popular Java IDEs so that developers write code, press the format keyboard shortcut and know that the output is acceptable for a patch. Eclipse calls them code formatter profiles and they can be exported and imported.
You raise a very good point and I agree that CSOFF isn't a practical
option. Having CSOFF everywhere in the code adds clutter and makes it
harder to read. We need to try and reach consenus on the checkstyle
rules, by abandoning some but not too many of the rules. Glenn, would
you be able to list the rules you object with in an order of priority,
with the ones that would inconvience you the most at the top of the list
with the least annoying ones at the bottom? I think that would help us
arrive at some sort of compromise.
>
> Related to line length, I would go for a maximum of 100. As already said, there is a limit on the amount of information that can be easily understood per line. More than this typically indicates methods with too many arguments or deep nesting that should be refactored into methods. Also, I really hate working with my laptop or going with it to a customer site for support and having to horizontally scroll in file diffs.
Thanks,
Chris
>
> Alexios Giotis
>
>
>
> On Feb 6, 2012, at 7:58 PM, Glenn Adams wrote:
>
>> 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<vh...@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
>>>
>>>
>
> ---------------------------------------------------------------------
> 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
Re: Checkstyle, Reloaded
Posted by Alexios Giotis <al...@gmail.com>.
Hi,
I can't see a point having checkstyle rules and then adding CSOFF on new files to disable them. It is faster to read, debug or fix source code when there is uniformity rather than every file having the personal style of the initial author. It would be helpful to additionally have configurations for popular Java IDEs so that developers write code, press the format keyboard shortcut and know that the output is acceptable for a patch. Eclipse calls them code formatter profiles and they can be exported and imported.
Related to line length, I would go for a maximum of 100. As already said, there is a limit on the amount of information that can be easily understood per line. More than this typically indicates methods with too many arguments or deep nesting that should be refactored into methods. Also, I really hate working with my laptop or going with it to a customer site for support and having to horizontally scroll in file diffs.
Alexios Giotis
On Feb 6, 2012, at 7:58 PM, Glenn Adams wrote:
> 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 <vh...@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
>>
>>
---------------------------------------------------------------------
To unsubscribe, e-mail: general-unsubscribe@xmlgraphics.apache.org
For additional commands, e-mail: general-help@xmlgraphics.apache.org
Re: Checkstyle, Reloaded
Posted by Glenn Adams <gl...@skynav.com>.
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 <vh...@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
>
>
Re: Checkstyle, Reloaded
Posted by Vincent Hennebert <vh...@gmail.com>.
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.
> 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 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.
> 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.
> 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.
> 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.
> 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 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.
> 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.
> 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 <vh...@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
Re: Checkstyle, Reloaded
Posted by Glenn Adams <gl...@skynav.com>.
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 <vh...@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
>
Re: Checkstyle, Reloaded
Posted by Glenn Adams <gl...@skynav.com>.
On Thu, Apr 26, 2012 at 9:19 AM, Vincent Hennebert <vh...@gmail.com>wrote:
> On 25/04/12 20:01, Glenn Adams wrote:
> > On Wed, Apr 25, 2012 at 12:29 PM, Vincent Hennebert <
> vhennebert@gmail.com>wrote:
> >
> >> On 25/04/12 19:03, Glenn Adams wrote:
> >>> On Wed, Apr 25, 2012 at 11:46 AM, Vincent Hennebert <
> >> vhennebert@gmail.com>wrote:
> >>>
> >>>> On 25/04/12 17:03, Glenn Adams wrote:
> >>>>> how does this differ from the current checkstyle-5.5.xml rules that
> are
> >>>> the
> >>>>> current default in fop?
> >>>>
> >>>> The following rules have been removed:
> >> <snip/>
> >>
> >>>> • CSOFF and CSOK
> >>>>
> >>>
> >>> i do not accept removing these unless you are willing to remove all
> rules
> >>> that trigger a warning/error in the absence of these pragmas
> >>
> >> Those are essentially the rules about whitespace. I’ve given reasons
> >> what I think we should keep some of them. Could you comment on them?
> >>
> >
> > i did; see my responses at [1-5]:
> >
> > [1] Re: Checkstyle,
> > Reloaded<
> http://mail-archives.apache.org/mod_mbox/xmlgraphics-general/201202.mbox/%3cCACQ=j+fuosd_5W09LDnNeCbo-rN+2kpsdqnbH752iH1-N+HJdQ@mail.gmail.com%3e
> >
> > [2] Re: Checkstyle,
> > Reloaded<
> http://mail-archives.apache.org/mod_mbox/xmlgraphics-general/201202.mbox/%3cCACQ=j+f6A+iuDhLYbqgGAmim-eqnMgyD3azvwQ0D8a6HH8bQkw@mail.gmail.com%3e
> >
> > [3] Re: Checkstyle,
> > Reloaded<
> http://mail-archives.apache.org/mod_mbox/xmlgraphics-general/201202.mbox/%3cCACQ=j+cEunN8_d0O=dUPCHmMsK9+71Pj3f4VYk23xZMrxuMuuA@mail.gmail.com%3e
> >
> > [4] Re: Checkstyle,
> > Reloaded<
> http://mail-archives.apache.org/mod_mbox/xmlgraphics-general/201202.mbox/%3cCACQ=j+c3ygYneGjUJP+6xXeMW4yS=79De=48xSZ=EqvuR0ofAw@mail.gmail.com%3e
> >
> > [5] Re: Checkstyle,
> > Reloaded<
> http://mail-archives.apache.org/mod_mbox/xmlgraphics-general/201202.mbox/%3cCACQ=j+fqPEePgSxkQ_c01vdAoN7scRcjytcHuw4FvbhzYBmvog@mail.gmail.com%3e
> >
>
> I saw that. What I would like to know is what you think about the
> readability concerns that have been raised?
>
please provide a link to whichever messages discusses those concerns
>
>
> <snip/>
> >>>> • ConstantName: removed log exception
> >>>>
> >>>
> >>> could you elaborate?
> >>
> >> Static final log fields will have to be made uppercase.
> >>
> >
> > I would prefer to leave it as is currently used.
>
> Why? We might as well convert them to the prescribed convention.
>
the current convention in FOP is lower case; you are proposing a new
convention; i would prefer to stay with the current convention for log
>
>
> <snip/>
> >>> i also don't accept changing LineLength back to 110; i believe
> >>> someone
> >>> proposed 130, which I can accept as long as i can disable entirely
> using
> >>> CSOFF; i would prefer *no* limit
> >>
> >> I (and others) have given good reasons why the line length should be
> >> limited. Surely those reasons prevail over mere style preference, don’t
> >> they?
> >>
> >
> > as i have stated numerous times, i use an editor (emacs) that makes long
> > lines easy to handle, so i don't have a problem with them; on my (15"
> > laptop) screen, i get 200 columns before a wrap; i prefer to *not* break
> a
> > statement artificially into lines simply due to an arbitrary line length
> > limit;
>
> Again this is a mere style preference. I’m afraid it doesn’t count
> compared to reasons of readability and convenience for side-by-side
> comparison.
>
sorry, but i disagree; you are arguing your convenience against my
convenience; you are going to insist that is more convenient for you to use
short lines, and i am going to insist that it is more convenient for me to
use long lines;
i proposed a compromise i can live with: 130 line length plus use of CSOFF
in files that do not follow this rule; i will not agree to anything less; i
would suggest you follow the advice given by Chris in [1]:
"I propose that we remove this rule as Glenn suggests and it will avoid lines
being broken in awkward places too."
[1]
http://mail-archives.apache.org/mod_mbox/xmlgraphics-general/201202.mbox/%3cBLU0-SMTP420D413C8764CC374545B5BFB7B0@phx.gbl%3e
>
>
> > if you don't mind me using my style in files i author (with CSOFF to
> > disable), then i can accept a shorter limit, e.g., i believe someone
> > proposed 130
>
> The goal is to remove CSOFF altogether. There’s no point having
> Checkstyle rules if anybody can disable them using CSOFF comments.
>
that may be your goal, but it is not my goal; if you want to insist on
applying a style rule that i disagree with in principle, then you will have
to allow for exceptions; a better approach would be to not insist on
applying any rule for which there is not unanimous agreement; however, i am
offering a compromise, which is that you may add a rule which i do no agree
with as long as you do not object to me disabling that rule in files i
author; you can't do both (apply the rule and rule out exclusions)
>
> Files your authored will sooner or later be read and modified by other
> people, so they shouldn’t receive any special treatment.
>
in that case, you should not insist on applying a style rule for which
there is not unanimous agreement
> I could agree to raise the limit to 120, but that’s the absolute
> maximum.
>
i can agree to one of the following regarding line length:
(1) default rule enforces 130, but allows exceptions via CSOFF/CSOK; [i
would point out that one of the most popular IBM 1403 printer, Model 2,
introduced in the early 60s and used with both IBM System/360 and
System/370 had 132 print positions]
or
(2) no rule for line length
>
>
> > but personally, i think it best not to enforce any limit
> >
> >
> >>
> >> Thanks,
> >> Vincent
> >>
> >>
> >>>>> On Wed, Apr 25, 2012 at 9:44 AM, Vincent Hennebert <
> >> vhennebert@gmail.com
> >>>>> wrote:
> >>>>>
> >>>>>> Ok, reviving a thread that has been dormant for too long.
> >>>>>>
> >>>>>> Attached is an updated version of the proposed Checkstyle
> >> configuration.
> >>>>>> I removed/relaxed the following rules:
> >>>>>> • EmptyBlock (allow comments)
> >>>>>> • ExplicitInitialization (not automatically fixable)
> >>>>>> • NoWhitespaceAfter with ARRAY_INIT token
> >>>>>> • ParenPad
> >>>>>>
> >>>>>> Note that I’m not happy with removing that last rule. I agree with
> >>>>>> Alexios that a consistent style makes reading and debugging easier.
> >> That
> >>>>>> wouldn’t be too bad if the original style were preserved in every
> >> source
> >>>>>> file, but this will clearly not happen. In fact, the mixing of
> styles
> >>>>>> has already started after the complex scripts patch was applied. I
> >> still
> >>>>>> removed the rule though.
> >>>>>>
> >>>>>> However, I left the MethodParamPad rule in order to remain compliant
> >>>>>> with Sun’s recommendations:
> >>>>>>
> >>>>>>
> >>>>
> >>
> http://www.oracle.com/technetwork/java/javase/documentation/codeconventions-141388.html#475
> >>>>>> I’d also like to keep the NoWhitespaceAfter rule, as whitespace
> after
> >>>>>> unary operators increases too much the risk of misreading the
> >> statement
> >>>>>> IMO.
> >>>>>>
> >>>>>> Finally, I left the LineLength rule to 110. Long lines impede code
> >>>>>> readability too much IMO. They also make side-by-side comparison
> >> harder.
> >>>>>> I note that some even recommend to leave the check to 100. I think
> 110
> >>>>>> should be an acceptable compromise.
> >>>>>>
> >>>>>> Please let me know what you think.
> >>>>>> Thanks,
> >>>>>> Vincent
>
>
> Vincent
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: general-unsubscribe@xmlgraphics.apache.org
> For additional commands, e-mail: general-help@xmlgraphics.apache.org
>
>
Re: Checkstyle, Reloaded
Posted by Vincent Hennebert <vh...@gmail.com>.
On 25/04/12 20:01, Glenn Adams wrote:
> On Wed, Apr 25, 2012 at 12:29 PM, Vincent Hennebert <vh...@gmail.com>wrote:
>
>> On 25/04/12 19:03, Glenn Adams wrote:
>>> On Wed, Apr 25, 2012 at 11:46 AM, Vincent Hennebert <
>> vhennebert@gmail.com>wrote:
>>>
>>>> On 25/04/12 17:03, Glenn Adams wrote:
>>>>> how does this differ from the current checkstyle-5.5.xml rules that are
>>>> the
>>>>> current default in fop?
>>>>
>>>> The following rules have been removed:
>> <snip/>
>>
>>>> • CSOFF and CSOK
>>>>
>>>
>>> i do not accept removing these unless you are willing to remove all rules
>>> that trigger a warning/error in the absence of these pragmas
>>
>> Those are essentially the rules about whitespace. I’ve given reasons
>> what I think we should keep some of them. Could you comment on them?
>>
>
> i did; see my responses at [1-5]:
>
> [1] Re: Checkstyle,
> Reloaded<http://mail-archives.apache.org/mod_mbox/xmlgraphics-general/201202.mbox/%3cCACQ=j+fuosd_5W09LDnNeCbo-rN+2kpsdqnbH752iH1-N+HJdQ@mail.gmail.com%3e>
> [2] Re: Checkstyle,
> Reloaded<http://mail-archives.apache.org/mod_mbox/xmlgraphics-general/201202.mbox/%3cCACQ=j+f6A+iuDhLYbqgGAmim-eqnMgyD3azvwQ0D8a6HH8bQkw@mail.gmail.com%3e>
> [3] Re: Checkstyle,
> Reloaded<http://mail-archives.apache.org/mod_mbox/xmlgraphics-general/201202.mbox/%3cCACQ=j+cEunN8_d0O=dUPCHmMsK9+71Pj3f4VYk23xZMrxuMuuA@mail.gmail.com%3e>
> [4] Re: Checkstyle,
> Reloaded<http://mail-archives.apache.org/mod_mbox/xmlgraphics-general/201202.mbox/%3cCACQ=j+c3ygYneGjUJP+6xXeMW4yS=79De=48xSZ=EqvuR0ofAw@mail.gmail.com%3e>
> [5] Re: Checkstyle,
> Reloaded<http://mail-archives.apache.org/mod_mbox/xmlgraphics-general/201202.mbox/%3cCACQ=j+fqPEePgSxkQ_c01vdAoN7scRcjytcHuw4FvbhzYBmvog@mail.gmail.com%3e>
I saw that. What I would like to know is what you think about the
readability concerns that have been raised?
<snip/>
>>>> • ConstantName: removed log exception
>>>>
>>>
>>> could you elaborate?
>>
>> Static final log fields will have to be made uppercase.
>>
>
> I would prefer to leave it as is currently used.
Why? We might as well convert them to the prescribed convention.
<snip/>
>>> i also don't accept changing LineLength back to 110; i believe
>>> someone
>>> proposed 130, which I can accept as long as i can disable entirely using
>>> CSOFF; i would prefer *no* limit
>>
>> I (and others) have given good reasons why the line length should be
>> limited. Surely those reasons prevail over mere style preference, don’t
>> they?
>>
>
> as i have stated numerous times, i use an editor (emacs) that makes long
> lines easy to handle, so i don't have a problem with them; on my (15"
> laptop) screen, i get 200 columns before a wrap; i prefer to *not* break a
> statement artificially into lines simply due to an arbitrary line length
> limit;
Again this is a mere style preference. I’m afraid it doesn’t count
compared to reasons of readability and convenience for side-by-side
comparison.
> if you don't mind me using my style in files i author (with CSOFF to
> disable), then i can accept a shorter limit, e.g., i believe someone
> proposed 130
The goal is to remove CSOFF altogether. There’s no point having
Checkstyle rules if anybody can disable them using CSOFF comments.
Files your authored will sooner or later be read and modified by other
people, so they shouldn’t receive any special treatment.
I could agree to raise the limit to 120, but that’s the absolute
maximum.
> but personally, i think it best not to enforce any limit
>
>
>>
>> Thanks,
>> Vincent
>>
>>
>>>>> On Wed, Apr 25, 2012 at 9:44 AM, Vincent Hennebert <
>> vhennebert@gmail.com
>>>>> wrote:
>>>>>
>>>>>> Ok, reviving a thread that has been dormant for too long.
>>>>>>
>>>>>> Attached is an updated version of the proposed Checkstyle
>> configuration.
>>>>>> I removed/relaxed the following rules:
>>>>>> • EmptyBlock (allow comments)
>>>>>> • ExplicitInitialization (not automatically fixable)
>>>>>> • NoWhitespaceAfter with ARRAY_INIT token
>>>>>> • ParenPad
>>>>>>
>>>>>> Note that I’m not happy with removing that last rule. I agree with
>>>>>> Alexios that a consistent style makes reading and debugging easier.
>> That
>>>>>> wouldn’t be too bad if the original style were preserved in every
>> source
>>>>>> file, but this will clearly not happen. In fact, the mixing of styles
>>>>>> has already started after the complex scripts patch was applied. I
>> still
>>>>>> removed the rule though.
>>>>>>
>>>>>> However, I left the MethodParamPad rule in order to remain compliant
>>>>>> with Sun’s recommendations:
>>>>>>
>>>>>>
>>>>
>> http://www.oracle.com/technetwork/java/javase/documentation/codeconventions-141388.html#475
>>>>>> I’d also like to keep the NoWhitespaceAfter rule, as whitespace after
>>>>>> unary operators increases too much the risk of misreading the
>> statement
>>>>>> IMO.
>>>>>>
>>>>>> Finally, I left the LineLength rule to 110. Long lines impede code
>>>>>> readability too much IMO. They also make side-by-side comparison
>> harder.
>>>>>> I note that some even recommend to leave the check to 100. I think 110
>>>>>> should be an acceptable compromise.
>>>>>>
>>>>>> Please let me know what you think.
>>>>>> Thanks,
>>>>>> Vincent
Vincent
---------------------------------------------------------------------
To unsubscribe, e-mail: general-unsubscribe@xmlgraphics.apache.org
For additional commands, e-mail: general-help@xmlgraphics.apache.org
Re: Checkstyle, Reloaded
Posted by Glenn Adams <gl...@skynav.com>.
On Wed, Apr 25, 2012 at 12:29 PM, Vincent Hennebert <vh...@gmail.com>wrote:
> On 25/04/12 19:03, Glenn Adams wrote:
> > On Wed, Apr 25, 2012 at 11:46 AM, Vincent Hennebert <
> vhennebert@gmail.com>wrote:
> >
> >> On 25/04/12 17:03, Glenn Adams wrote:
> >>> how does this differ from the current checkstyle-5.5.xml rules that are
> >> the
> >>> current default in fop?
> >>
> >> The following rules have been removed:
> <snip/>
>
> >> • CSOFF and CSOK
> >>
> >
> > i do not accept removing these unless you are willing to remove all rules
> > that trigger a warning/error in the absence of these pragmas
>
> Those are essentially the rules about whitespace. I’ve given reasons
> what I think we should keep some of them. Could you comment on them?
>
i did; see my responses at [1-5]:
[1] Re: Checkstyle,
Reloaded<http://mail-archives.apache.org/mod_mbox/xmlgraphics-general/201202.mbox/%3cCACQ=j+fuosd_5W09LDnNeCbo-rN+2kpsdqnbH752iH1-N+HJdQ@mail.gmail.com%3e>
[2] Re: Checkstyle,
Reloaded<http://mail-archives.apache.org/mod_mbox/xmlgraphics-general/201202.mbox/%3cCACQ=j+f6A+iuDhLYbqgGAmim-eqnMgyD3azvwQ0D8a6HH8bQkw@mail.gmail.com%3e>
[3] Re: Checkstyle,
Reloaded<http://mail-archives.apache.org/mod_mbox/xmlgraphics-general/201202.mbox/%3cCACQ=j+cEunN8_d0O=dUPCHmMsK9+71Pj3f4VYk23xZMrxuMuuA@mail.gmail.com%3e>
[4] Re: Checkstyle,
Reloaded<http://mail-archives.apache.org/mod_mbox/xmlgraphics-general/201202.mbox/%3cCACQ=j+c3ygYneGjUJP+6xXeMW4yS=79De=48xSZ=EqvuR0ofAw@mail.gmail.com%3e>
[5] Re: Checkstyle,
Reloaded<http://mail-archives.apache.org/mod_mbox/xmlgraphics-general/201202.mbox/%3cCACQ=j+fqPEePgSxkQ_c01vdAoN7scRcjytcHuw4FvbhzYBmvog@mail.gmail.com%3e>
>
>
> >> • Double (No idea what it is about. It doesn’t appear in the list of
> >> available checks for Checkstyle 5.5.)
> >>
> >
> > the full name is DoubleCheckedLocking, which is documented at
> >
> >
> http://checkstyle.sourceforge.net/config_coding.html#DoubleCheckedLocking
>
> Ha, ok. I think it’s not Checkstyle’s job to check for that.
>
>
> <snip/>
> >> • EqualsHashCode
> >>
> >
> > i think this should stay, since it is part of the object contract, and
> > exceptions (via CSOFF/CSOK) need to be explicitly documented
>
> Same here. I think Checkstyle should be restricted to, well, checking
> style.
>
>
> <snip/>
> >> • ConstantName: removed log exception
> >>
> >
> > could you elaborate?
>
> Static final log fields will have to be made uppercase.
>
I would prefer to leave it as is currently used.
> >> • WhitespaceAfter: added typecast to follow Sun’s conventions
> >>
> >
> > i don't accept this, particularly since it is widely used in FOP code
> (and
> > I always use whitespace after typecast)
>
> ?? Using a whitespace after a cast is precisely what this rule enforces.
ah, then i guess i noticed many existing uses that did not put whitespace
after the typecast; if you wish to enforce this and also will make the
changes to existing code, then i can agree
>
> > i also don't accept changing LineLength back to 110; i believe someone
> > proposed 130, which I can accept as long as i can disable entirely using
> > CSOFF; i would prefer *no* limit
>
> I (and others) have given good reasons why the line length should be
> limited. Surely those reasons prevail over mere style preference, don’t
> they?
>
as i have stated numerous times, i use an editor (emacs) that makes long
lines easy to handle, so i don't have a problem with them; on my (15"
laptop) screen, i get 200 columns before a wrap; i prefer to *not* break a
statement artificially into lines simply due to an arbitrary line length
limit;
if you don't mind me using my style in files i author (with CSOFF to
disable), then i can accept a shorter limit, e.g., i believe someone
proposed 130
but personally, i think it best not to enforce any limit
>
> Thanks,
> Vincent
>
>
> >>> On Wed, Apr 25, 2012 at 9:44 AM, Vincent Hennebert <
> vhennebert@gmail.com
> >>> wrote:
> >>>
> >>>> Ok, reviving a thread that has been dormant for too long.
> >>>>
> >>>> Attached is an updated version of the proposed Checkstyle
> configuration.
> >>>> I removed/relaxed the following rules:
> >>>> • EmptyBlock (allow comments)
> >>>> • ExplicitInitialization (not automatically fixable)
> >>>> • NoWhitespaceAfter with ARRAY_INIT token
> >>>> • ParenPad
> >>>>
> >>>> Note that I’m not happy with removing that last rule. I agree with
> >>>> Alexios that a consistent style makes reading and debugging easier.
> That
> >>>> wouldn’t be too bad if the original style were preserved in every
> source
> >>>> file, but this will clearly not happen. In fact, the mixing of styles
> >>>> has already started after the complex scripts patch was applied. I
> still
> >>>> removed the rule though.
> >>>>
> >>>> However, I left the MethodParamPad rule in order to remain compliant
> >>>> with Sun’s recommendations:
> >>>>
> >>>>
> >>
> http://www.oracle.com/technetwork/java/javase/documentation/codeconventions-141388.html#475
> >>>> I’d also like to keep the NoWhitespaceAfter rule, as whitespace after
> >>>> unary operators increases too much the risk of misreading the
> statement
> >>>> IMO.
> >>>>
> >>>> Finally, I left the LineLength rule to 110. Long lines impede code
> >>>> readability too much IMO. They also make side-by-side comparison
> harder.
> >>>> I note that some even recommend to leave the check to 100. I think 110
> >>>> should be an acceptable compromise.
> >>>>
> >>>> Please let me know what you think.
> >>>> Thanks,
> >>>> Vincent
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: general-unsubscribe@xmlgraphics.apache.org
> For additional commands, e-mail: general-help@xmlgraphics.apache.org
>
>
Re: Checkstyle, Reloaded
Posted by Vincent Hennebert <vh...@gmail.com>.
On 25/04/12 19:03, Glenn Adams wrote:
> On Wed, Apr 25, 2012 at 11:46 AM, Vincent Hennebert <vh...@gmail.com>wrote:
>
>> On 25/04/12 17:03, Glenn Adams wrote:
>>> how does this differ from the current checkstyle-5.5.xml rules that are
>> the
>>> current default in fop?
>>
>> The following rules have been removed:
<snip/>
>> • CSOFF and CSOK
>>
>
> i do not accept removing these unless you are willing to remove all rules
> that trigger a warning/error in the absence of these pragmas
Those are essentially the rules about whitespace. I’ve given reasons
what I think we should keep some of them. Could you comment on them?
>> • Double (No idea what it is about. It doesn’t appear in the list of
>> available checks for Checkstyle 5.5.)
>>
>
> the full name is DoubleCheckedLocking, which is documented at
>
> http://checkstyle.sourceforge.net/config_coding.html#DoubleCheckedLocking
Ha, ok. I think it’s not Checkstyle’s job to check for that.
<snip/>
>> • EqualsHashCode
>>
>
> i think this should stay, since it is part of the object contract, and
> exceptions (via CSOFF/CSOK) need to be explicitly documented
Same here. I think Checkstyle should be restricted to, well, checking
style.
<snip/>
>> • ConstantName: removed log exception
>>
>
> could you elaborate?
Static final log fields will have to be made uppercase.
>> • WhitespaceAfter: added typecast to follow Sun’s conventions
>>
>
> i don't accept this, particularly since it is widely used in FOP code (and
> I always use whitespace after typecast)
?? Using a whitespace after a cast is precisely what this rule enforces.
> i also don't accept changing LineLength back to 110; i believe someone
> proposed 130, which I can accept as long as i can disable entirely using
> CSOFF; i would prefer *no* limit
I (and others) have given good reasons why the line length should be
limited. Surely those reasons prevail over mere style preference, don’t
they?
Thanks,
Vincent
>>> On Wed, Apr 25, 2012 at 9:44 AM, Vincent Hennebert <vhennebert@gmail.com
>>> wrote:
>>>
>>>> Ok, reviving a thread that has been dormant for too long.
>>>>
>>>> Attached is an updated version of the proposed Checkstyle configuration.
>>>> I removed/relaxed the following rules:
>>>> • EmptyBlock (allow comments)
>>>> • ExplicitInitialization (not automatically fixable)
>>>> • NoWhitespaceAfter with ARRAY_INIT token
>>>> • ParenPad
>>>>
>>>> Note that I’m not happy with removing that last rule. I agree with
>>>> Alexios that a consistent style makes reading and debugging easier. That
>>>> wouldn’t be too bad if the original style were preserved in every source
>>>> file, but this will clearly not happen. In fact, the mixing of styles
>>>> has already started after the complex scripts patch was applied. I still
>>>> removed the rule though.
>>>>
>>>> However, I left the MethodParamPad rule in order to remain compliant
>>>> with Sun’s recommendations:
>>>>
>>>>
>> http://www.oracle.com/technetwork/java/javase/documentation/codeconventions-141388.html#475
>>>> I’d also like to keep the NoWhitespaceAfter rule, as whitespace after
>>>> unary operators increases too much the risk of misreading the statement
>>>> IMO.
>>>>
>>>> Finally, I left the LineLength rule to 110. Long lines impede code
>>>> readability too much IMO. They also make side-by-side comparison harder.
>>>> I note that some even recommend to leave the check to 100. I think 110
>>>> should be an acceptable compromise.
>>>>
>>>> Please let me know what you think.
>>>> Thanks,
>>>> Vincent
---------------------------------------------------------------------
To unsubscribe, e-mail: general-unsubscribe@xmlgraphics.apache.org
For additional commands, e-mail: general-help@xmlgraphics.apache.org
Re: Checkstyle, Reloaded
Posted by Glenn Adams <gl...@skynav.com>.
On Wed, Apr 25, 2012 at 11:46 AM, Vincent Hennebert <vh...@gmail.com>wrote:
> On 25/04/12 17:03, Glenn Adams wrote:
> > how does this differ from the current checkstyle-5.5.xml rules that are
> the
> > current default in fop?
>
> The following rules have been removed:
> • prohibiting the usage of @author but we can add it back
>
i'm fine with keeping this in, since I already removed all existing usage
of @author (in FOP files)
> • CSOFF and CSOK
>
i do not accept removing these unless you are willing to remove all rules
that trigger a warning/error in the absence of these pragmas
> • Double (No idea what it is about. It doesn’t appear in the list of
> available checks for Checkstyle 5.5.)
>
the full name is DoubleCheckedLocking, which is documented at
http://checkstyle.sourceforge.net/config_coding.html#DoubleCheckedLocking
> • FileContentsHolder (same)
>
this is needed for CSOFF/CSOK to work
> • InnerAssignments
>
i don't mind removing this, particularly since I use inner assignments
(with CSOFF/CSOK as needed)
> • EqualsHashCode
>
i think this should stay, since it is part of the object contract, and
exceptions (via CSOFF/CSOK) need to be explicitly documented
>
> The following rules have been modified:
> • AvoidStarImport: severity changed from error to warning
>
ok
> • ConstantName: removed log exception
>
could you elaborate?
> • WhitespaceAfter: added typecast to follow Sun’s conventions
>
i don't accept this, particularly since it is widely used in FOP code (and
I always use whitespace after typecast)
i also don't accept changing LineLength back to 110; i believe someone
proposed 130, which I can accept as long as i can disable entirely using
CSOFF; i would prefer *no* limit
>
>
> Vincent
>
>
> > On Wed, Apr 25, 2012 at 9:44 AM, Vincent Hennebert <vhennebert@gmail.com
> >wrote:
> >
> >> Ok, reviving a thread that has been dormant for too long.
> >>
> >> Attached is an updated version of the proposed Checkstyle configuration.
> >> I removed/relaxed the following rules:
> >> • EmptyBlock (allow comments)
> >> • ExplicitInitialization (not automatically fixable)
> >> • NoWhitespaceAfter with ARRAY_INIT token
> >> • ParenPad
> >>
> >> Note that I’m not happy with removing that last rule. I agree with
> >> Alexios that a consistent style makes reading and debugging easier. That
> >> wouldn’t be too bad if the original style were preserved in every source
> >> file, but this will clearly not happen. In fact, the mixing of styles
> >> has already started after the complex scripts patch was applied. I still
> >> removed the rule though.
> >>
> >> However, I left the MethodParamPad rule in order to remain compliant
> >> with Sun’s recommendations:
> >>
> >>
> http://www.oracle.com/technetwork/java/javase/documentation/codeconventions-141388.html#475
> >> I’d also like to keep the NoWhitespaceAfter rule, as whitespace after
> >> unary operators increases too much the risk of misreading the statement
> >> IMO.
> >>
> >> Finally, I left the LineLength rule to 110. Long lines impede code
> >> readability too much IMO. They also make side-by-side comparison harder.
> >> I note that some even recommend to leave the check to 100. I think 110
> >> should be an acceptable compromise.
> >>
> >> Please let me know what you think.
> >> Thanks,
> >> Vincent
> >>
> >>
> >> On 03/02/12 17:45, Vincent Hennebert 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
>
>
Re: Checkstyle, Reloaded
Posted by Vincent Hennebert <vh...@gmail.com>.
On 25/04/12 17:03, Glenn Adams wrote:
> how does this differ from the current checkstyle-5.5.xml rules that are the
> current default in fop?
The following rules have been removed:
• prohibiting the usage of @author but we can add it back
• CSOFF and CSOK
• Double (No idea what it is about. It doesn’t appear in the list of
available checks for Checkstyle 5.5.)
• FileContentsHolder (same)
• InnerAssignments
• EqualsHashCode
The following rules have been modified:
• AvoidStarImport: severity changed from error to warning
• ConstantName: removed log exception
• WhitespaceAfter: added typecast to follow Sun’s conventions
Vincent
> On Wed, Apr 25, 2012 at 9:44 AM, Vincent Hennebert <vh...@gmail.com>wrote:
>
>> Ok, reviving a thread that has been dormant for too long.
>>
>> Attached is an updated version of the proposed Checkstyle configuration.
>> I removed/relaxed the following rules:
>> • EmptyBlock (allow comments)
>> • ExplicitInitialization (not automatically fixable)
>> • NoWhitespaceAfter with ARRAY_INIT token
>> • ParenPad
>>
>> Note that I’m not happy with removing that last rule. I agree with
>> Alexios that a consistent style makes reading and debugging easier. That
>> wouldn’t be too bad if the original style were preserved in every source
>> file, but this will clearly not happen. In fact, the mixing of styles
>> has already started after the complex scripts patch was applied. I still
>> removed the rule though.
>>
>> However, I left the MethodParamPad rule in order to remain compliant
>> with Sun’s recommendations:
>>
>> http://www.oracle.com/technetwork/java/javase/documentation/codeconventions-141388.html#475
>> I’d also like to keep the NoWhitespaceAfter rule, as whitespace after
>> unary operators increases too much the risk of misreading the statement
>> IMO.
>>
>> Finally, I left the LineLength rule to 110. Long lines impede code
>> readability too much IMO. They also make side-by-side comparison harder.
>> I note that some even recommend to leave the check to 100. I think 110
>> should be an acceptable compromise.
>>
>> Please let me know what you think.
>> Thanks,
>> Vincent
>>
>>
>> On 03/02/12 17:45, Vincent Hennebert 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
Re: Checkstyle, Reloaded
Posted by Glenn Adams <gl...@skynav.com>.
how does this differ from the current checkstyle-5.5.xml rules that are the
current default in fop?
On Wed, Apr 25, 2012 at 9:44 AM, Vincent Hennebert <vh...@gmail.com>wrote:
> Ok, reviving a thread that has been dormant for too long.
>
> Attached is an updated version of the proposed Checkstyle configuration.
> I removed/relaxed the following rules:
> • EmptyBlock (allow comments)
> • ExplicitInitialization (not automatically fixable)
> • NoWhitespaceAfter with ARRAY_INIT token
> • ParenPad
>
> Note that I’m not happy with removing that last rule. I agree with
> Alexios that a consistent style makes reading and debugging easier. That
> wouldn’t be too bad if the original style were preserved in every source
> file, but this will clearly not happen. In fact, the mixing of styles
> has already started after the complex scripts patch was applied. I still
> removed the rule though.
>
> However, I left the MethodParamPad rule in order to remain compliant
> with Sun’s recommendations:
>
> http://www.oracle.com/technetwork/java/javase/documentation/codeconventions-141388.html#475
> I’d also like to keep the NoWhitespaceAfter rule, as whitespace after
> unary operators increases too much the risk of misreading the statement
> IMO.
>
> Finally, I left the LineLength rule to 110. Long lines impede code
> readability too much IMO. They also make side-by-side comparison harder.
> I note that some even recommend to leave the check to 100. I think 110
> should be an acceptable compromise.
>
> Please let me know what you think.
> Thanks,
> Vincent
>
>
> On 03/02/12 17:45, Vincent Hennebert 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
>
Re: Checkstyle, Reloaded
Posted by Vincent Hennebert <vh...@gmail.com>.
Ok, reviving a thread that has been dormant for too long.
Attached is an updated version of the proposed Checkstyle configuration.
I removed/relaxed the following rules:
• EmptyBlock (allow comments)
• ExplicitInitialization (not automatically fixable)
• NoWhitespaceAfter with ARRAY_INIT token
• ParenPad
Note that I’m not happy with removing that last rule. I agree with
Alexios that a consistent style makes reading and debugging easier. That
wouldn’t be too bad if the original style were preserved in every source
file, but this will clearly not happen. In fact, the mixing of styles
has already started after the complex scripts patch was applied. I still
removed the rule though.
However, I left the MethodParamPad rule in order to remain compliant
with Sun’s recommendations:
http://www.oracle.com/technetwork/java/javase/documentation/codeconventions-141388.html#475
I’d also like to keep the NoWhitespaceAfter rule, as whitespace after
unary operators increases too much the risk of misreading the statement
IMO.
Finally, I left the LineLength rule to 110. Long lines impede code
readability too much IMO. They also make side-by-side comparison harder.
I note that some even recommend to leave the check to 100. I think 110
should be an acceptable compromise.
Please let me know what you think.
Thanks,
Vincent
On 03/02/12 17:45, Vincent Hennebert 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
Re: Checkstyle, Reloaded
Posted by Chris Bowditch <bo...@hotmail.com>.
On 08/02/2012 20:51, Vincent Hennebert wrote:
> Hi Chris,
Hi Vincent,
> <snip/>
>> Can you provide a breakdown of the new warnings identified by Glenn by rule
>> type? I do object to introducing so many new warnings and I'm not comfortable
>> with using automated tools to correct the files, without understanding exactly
>> which warnings will be fixed in an automated way.
> Here is the list of rule violations that I get when running the new
> Checkstyle file on the latest trunk with Checkstyle 5.5:
> 1 FinalClassCheck
> 1 GenericWhitespaceCheck
> 1 NoWhitespaceBeforeCheck
> 2 DefaultComesLastCheck
> 4 RedundantModifierCheck
> 4 RightCurlyCheck
> 5 OneStatementPerLineCheck
> 8 RedundantImportCheck
> 8 RegexpSinglelineCheck
> 33 ConstantNameCheck
> 37 MultipleVariableDeclarationsCheck
> 47 UnusedImportsCheck
> 71 EmptyBlockCheck
> 113 NewlineAtEndOfFileCheck
> 128 NoWhitespaceAfterCheck
> 182 LineLengthCheck
> 249 ImportOrderCheck
> 321 ParenPadCheck
> 392 MethodParamPadCheck
> 806 ExplicitInitializationCheck
> 2231 WhitespaceAfterCheck
Thanks for preparing the breakdown. I've reviewed them and agree with
your analysis. Most of the ones that occur frequently are easily fixed
by the Eclipse Formatter, with the exceptions you've raised below.
Although the formatter may break long lines at unfavourable places? I
propose that we remove this rule as Glenn suggests and it will avoid
lines being broken in awkward places too.
Thanks,
Chris
>
> A description of the rules sorted by alphabetic order can be found here:
> http://checkstyle.sourceforge.net/availablechecks.html
>
> ImportOrderCheck and UnusedImportsCheck are easily fixed by bulk-running
> Eclipse’s import organizer.
>
> ConstantNameCheck and MultipleVariableDeclarationsCheck will have to be
> fixed by hand but the number remains reasonable.
>
> ExplicitInitializationCheck can’t be automatically fixed by Eclipse
> AFAICT. We may have to drop this rule as fixing it manually would be too
> much work.
>
> For the rest, either they are automatically fixed by the code formatter,
> or the number of occurrences is small enough to be manageable by hand.
>
>
> 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
Re: Checkstyle, Reloaded
Posted by Glenn Adams <gl...@skynav.com>.
On Wed, Feb 8, 2012 at 1:51 PM, Vincent Hennebert <vh...@gmail.com>wrote:
> 1 FinalClassCheck
> 1 GenericWhitespaceCheck
> 1 NoWhitespaceBeforeCheck
> 2 DefaultComesLastCheck
> 4 RedundantModifierCheck
> 4 RightCurlyCheck
> 5 OneStatementPerLineCheck
> 8 RedundantImportCheck
> 8 RegexpSinglelineCheck
> 33 ConstantNameCheck
> 37 MultipleVariableDeclarationsCheck
> 47 UnusedImportsCheck
> 71 EmptyBlockCheck
> 113 NewlineAtEndOfFileCheck
> 128 NoWhitespaceAfterCheck
> 182 LineLengthCheck
> 249 ImportOrderCheck
> 321 ParenPadCheck
> 392 MethodParamPadCheck
> 806 ExplicitInitializationCheck
> 2231 WhitespaceAfterCheck
>
Of these, I have problems with the following rules:
MethodParamPadCheck: OK if <property name="option" value="space"/>;
otherwise, should remove check
ParenPadCheck: OK if <property name="option" value="space"/>; otherwise,
should remove check
LineLengthCheck: OK if <property name="max" value="150"/>; otherwise,
should remove check
EmptyBlockCheck: OK if <property name="option" value="text"/>; otherwise,
should remove check
MultipleVariableDeclarationsCheck: NOT OK
If the intention is to have all code follow the same rules without use of
CSOFF declarations, I need to further verify these rules with the suggested
changes on my i18n dev branch.
Re: Checkstyle, Reloaded
Posted by Vincent Hennebert <vh...@gmail.com>.
Hi Chris,
On 06/02/12 10:14, Chris Bowditch wrote:
> On 03/02/2012 17:45, Vincent Hennebert wrote:
<snip/>
>>
>> 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.
> I'm not comfortable with that. Automated tools don't always fully grasp the
> code that they are changing, so there is a risk of introducing regressions.
I understand and share your concern in principle, however I believe that
in the present case the tool would be harmless. It really is as if
I were opening every Java file in Eclipse one by one and running the
formatter (aka hitting the Ctrl-F key). I actually discovered that I can
run the formatter on a whole source folder straight from the Eclipse
GUI. So I don’t even need to run a command-line tool. I would just
carefully configure the formatter and run it once on src/java and once
on test/java, and that would be it. How does that sound?
<snip/>
> Can you provide a breakdown of the new warnings identified by Glenn by rule
> type? I do object to introducing so many new warnings and I'm not comfortable
> with using automated tools to correct the files, without understanding exactly
> which warnings will be fixed in an automated way.
Here is the list of rule violations that I get when running the new
Checkstyle file on the latest trunk with Checkstyle 5.5:
1 FinalClassCheck
1 GenericWhitespaceCheck
1 NoWhitespaceBeforeCheck
2 DefaultComesLastCheck
4 RedundantModifierCheck
4 RightCurlyCheck
5 OneStatementPerLineCheck
8 RedundantImportCheck
8 RegexpSinglelineCheck
33 ConstantNameCheck
37 MultipleVariableDeclarationsCheck
47 UnusedImportsCheck
71 EmptyBlockCheck
113 NewlineAtEndOfFileCheck
128 NoWhitespaceAfterCheck
182 LineLengthCheck
249 ImportOrderCheck
321 ParenPadCheck
392 MethodParamPadCheck
806 ExplicitInitializationCheck
2231 WhitespaceAfterCheck
A description of the rules sorted by alphabetic order can be found here:
http://checkstyle.sourceforge.net/availablechecks.html
ImportOrderCheck and UnusedImportsCheck are easily fixed by bulk-running
Eclipse’s import organizer.
ConstantNameCheck and MultipleVariableDeclarationsCheck will have to be
fixed by hand but the number remains reasonable.
ExplicitInitializationCheck can’t be automatically fixed by Eclipse
AFAICT. We may have to drop this rule as fixing it manually would be too
much work.
For the rest, either they are automatically fixed by the code formatter,
or the number of occurrences is small enough to be manageable by hand.
Vincent
---------------------------------------------------------------------
To unsubscribe, e-mail: general-unsubscribe@xmlgraphics.apache.org
For additional commands, e-mail: general-help@xmlgraphics.apache.org
Re: Checkstyle, Reloaded
Posted by Chris Bowditch <bo...@hotmail.com>.
On 03/02/2012 17:45, Vincent Hennebert wrote:
> Hi All,
Hi Vincent,
>
> 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.
Agreed.
>
> 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.
Sounds good so far.
>
> 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.
I'm not comfortable with that. Automated tools don't always fully grasp
the code that they are changing, so there is a risk of introducing
regressions.
>
> 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.
Can you provide a breakdown of the new warnings identified by Glenn by
rule type? I do object to introducing so many new warnings and I'm not
comfortable with using automated tools to correct the files, without
understanding exactly which warnings will be fixed in an automated way.
Thanks,
Chris
>
> 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
Re: Checkstyle, Reloaded
Posted by Pascal Sancho <pa...@takoma.fr>.
Hi,
rather than display capabilities, we should consider how human can
interpret a long line.
It is established that he can read and understand 10 entities (words or
signs) at a glance.
Coding usages give a useful line width about 75-85 characters.
We should add to this an average indent. Personally, I use 95 chars as
max length, and I'm happy with this, even in side-by-side display
(modern IDE have automatic horizontal scroll, so deep indent is not a
problem here).
Le 03/02/2012 18:45, Vincent Hennebert a écrit :
> 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.
--
Pascal
---------------------------------------------------------------------
To unsubscribe, e-mail: general-unsubscribe@xmlgraphics.apache.org
For additional commands, e-mail: general-help@xmlgraphics.apache.org