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
> <vhenneb...@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 <vhenneb...@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-unsubscr...@xmlgraphics.apache.org
>>>> For additional commands, e-mail: general-h...@xmlgraphics.apache.org
>>>>
>>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: general-unsubscr...@xmlgraphics.apache.org
>> For additional commands, e-mail: general-h...@xmlgraphics.apache.org
>>
>>
---------------------------------------------------------------------
To unsubscribe, e-mail: general-unsubscr...@xmlgraphics.apache.org
For additional commands, e-mail: general-h...@xmlgraphics.apache.org
