xmlgraphics-general mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Vincent Hennebert <vhenneb...@gmail.com>
Subject Re: Checkstyle, Reloaded
Date Wed, 25 Apr 2012 18:29:16 GMT
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?


>> • 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


Mime
View raw message