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 17:46:19 GMT
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 <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


Mime
View raw message