OK, I understand your approach now. Makes sense. The discussion on
general@ can continue in parallel then.

I’m OK with progressively fixing the warnings in different sets of
commits and I agree with your proposed list. Will you also fix the
tests? Eventually we want to check them as well.

I’d be happy to fix RegexpSingleLine and NewLineAtEndOfFile if that
helps.

Thanks for looking into this!

Vincent


On 02/03/12 17:27, Glenn Adams wrote:
> I think we will need to come to an agreement (on the final set of 5.5
> rules) incrementally rather than all at once. Furthermore, implementing the
> changes to accommodate the new rules will also (most likely) occur
> incrementally.
> 
> While this is being done, it is useful to not disturb the checkstyle usage
> of others, so it makes sense to parameterize the selected configuration.
> 
> While the changes are being made for support the new rules, it also makes
> it easier to switch back and forth between the new and old rules, and
> ensure that the code changes aren't breaking on either rule set.
> 
> Once all the changes are effected, then we can change the default config to
> the new 5.5 rules.
> 
> In the future, we can use the same parameterization to help transition to
> newer versions of checkstyle, 5.6, 5.7, etc.
> 
> So having a checkstyle.config parameter turns out to be very useful.
> 
> Now that I have put this mechanism in place, we can proceed with making the
> code changes to accommodate the new rules, and we can divide up that task
> between us and others can join as well if they wish.
> 
> As Chris mentioned previously, there are risks involved in making
> auto-fixes, so I prefer to use manual edits for the changes.
> 
> If you don't mind, I'd like to start effecting the changes needed for
> fixing the rules involving 300 or less new errors as shown in the report
> below. I would address these in multiple batches (commits) to make it
> easier to manage the changes, e.g., one commit per rule. Specifically, I
> would start with:
> 
>> ImportOrder                     255
>> NoWhitespaceAfter               183
>> NewLineAtEndOfFile              111
>> UnusedImports                   80
>> MultipleVariableDeclarations    78
>> RegexpSingleLine                46
>> OneStatementPerLine             43
>> RedundantImport                 22
>> DefaultComesLast                9
>> RightCurly                      5
>> RedundantModifier               3
>> GenericWhitespace               1
>> NoWhitespaceBefore              1
> 
> Are there any objections on my proceeding with this?
> 
> On Fri, Mar 2, 2012 at 4:30 AM, Vincent Hennebert <vhenneb...@gmail.com>wrote:
> 
>> Please can we wait to reach an agreement before making any changes?
>> I realize the thread in @general hasn’t made any progress in 3 weeks and
>> I’ll try to revive it, but making changes now will just make things more
>> confusing.
>>
>> In particular, I think we want to all use the same version of Checkstyle
>> in order to ensure consistency. ATM Checkstyle 5.1 is used but when the
>> new set of rules is in place I think we will all want to switch to
>> Checkstyle 5.5.
>>
>> The checkstyle.location however is a good move, I like it.
>>
>> Thanks,
>> Vincent
>>
>>
>> On 02/03/12 00:30, Glenn Adams wrote:
>>> In order to proceed on Vincent's proposed new checkstyle rules [1], I've
>>> committed support for checkstyle-5.5 and the new rules [2].
>>>
>>> [1]
>>>
>> http://mail-archives.apache.org/mod_mbox/xmlgraphics-general/201202.mbox/%3c4f2c1d25.8010...@gmail.com%3e
>>> [2] http://svn.apache.org/viewvc?view=revision&revision=1296000
>>>
>>> More specifically, I parameterized build.xml to use either checkstyle-5.1
>>> or checkstyle-5.5, where the (current) default is checkstyle-5.1. Which
>>> version is used can be controlled by setting the following ant
>> parameters:
>>>
>>>    - checkstyle.location (default: ${lib-tools}/checkstyle-all-5.1.jar)
>>>    - checkstyle.config (default: ${basedir}/checkstyle-5.1.xml)
>>>
>>> I'm specifying the following in my build-local.properties in order to
>>> select checkstyle-5.5:
>>>
>>> checkstyle.config = ${basedir}/checkstyle-5.5.xml
>>> checkstyle.location = ${basedir}/lib/build/checkstyle-5.5-all.jar
>>>
>>> I manually merged the new rules proposed by Vincent with the old set of
>>> checkstyle-5.1 rules to produce a new checkstyle-5.5.xml config file as
>>> follows:
>>>
>>>    - if adding a new rule did not produce new errors, i added;
>> otherwise, i
>>>    added a commented out rule with a comment indicating the number of new
>>>    errors
>>>    - if changing an existing rule did not produce new errors, i changed
>> it;
>>>    otherwise, i added a comment indicating the number of new errors
>> produced
>>>    - for existing rules not included in [1], I removed some but retained
>> a
>>>    few, about which see the additional notes below;
>>>    - i reordered the rules to be alphabetical, and added some divider
>>>    comments to make it a little easier to read the separate rules
>>>
>>> As of this commit, both checkstyle-5.1 and -5.5 do not produce any
>> errors;
>>> however, in order to make full use of all the proposed new rules or
>> changed
>>> rules some code will need to change or the proposed new rule or rule
>> change
>>> dropped. An ordered list of the number of new errors that would be
>> produced
>>> (by rule) is as follows:
>>>
>>> ParenPad                        16666
>>> MethodParamPad                  4316
>>> WhitespaceAfter                 2203
>>> ExplicitInitialization          795
>>> ImportOrder                     255
>>> NoWhitespaceAfter               183
>>> NewLineAtEndOfFile              111
>>> UnusedImports                   80
>>> MultipleVariableDeclarations    78
>>> RegexpSingleLine                46
>>> OneStatementPerLine             43
>>> RedundantImport                 22
>>> DefaultComesLast                9
>>> RightCurly                      5
>>> RedundantModifier               3
>>> GenericWhitespace               1
>>> NoWhitespaceBefore              1
>>>
>>> For those rules above with less than, say 300 errors, the errors can be
>>> addressed (in subsequent commits) and the rule enabled. For those rules
>>> above producing more than 500 new errors, I would suggest we not attempt
>> to
>>> implement the rules, particularly since we don't have full consensus on
>>> their use.
>>>
>>> Here are my notes on the rule merge process:
>>>
>>> ConstantName
>>>   1. current format ^([A-Z](_?[A-Z0-9]+)*)|(log)$
>>>   2. proposed (default) format ^[A-Z][A-Z0-9]*(_[A-Z0-9]+)*$
>>>   3. problems with current code base
>>>      (a) requires at least one '_'
>>>      (b) requires length > 1
>>>      (b) doesn't admit 'log'
>>>   4. conclusion: retain existing format
>>>
>>> DefaultComesLast
>>>   1. produces 9 new errors
>>>
>>> DoubleCheckedLockinng
>>>   1. left in, even though not present in proposed rules
>>>
>>> EmptyBlock
>>>   1. left LITERAL_CATCH, even though removed in proposed rule
>>>   2. changing block policy to default (stmt) instead of text produces 110
>>> new errors
>>>
>>> EmptyForIteratorPad
>>>   1. removed (not present in proposed rules)
>>>
>>> EqualsHashCode
>>>   1. left in, even though not present in proposed rules
>>>
>>> ExplicitInitialization
>>>   1. produces 795 new errors
>>>
>>> FileLength
>>>   1. removed (not present in proposed rules)
>>>
>>> GenericWhitespace
>>>   1. produces 1 new error
>>>
>>> ImportOrder
>>>   1. produces 255 new errors
>>>
>>> InnerAssignment
>>>   1. left in, even though not present in proposed rules (I propose we
>>> remove this rule, but want to confirm with the group before doing so)
>>>
>>> Javadoc{Method,Type,Variable}
>>>   1. removed (not present in proposed rules)
>>>
>>> LineLength
>>>   1. removed, as suggested by cbowditch [3]
>>>
>>> [3]
>>>
>> http://mail-archives.apache.org/mod_mbox/xmlgraphics-general/201202.mbox/%3cblu0-smtp420d413c8764cc374545b5bfb...@phx.gbl%3e
>>>
>>> MethodLength
>>>   1. removed (not present in proposed rules)
>>>
>>> MethodParamPad
>>>   1. produces 4316 new errors
>>>
>>> MultipleVariableDeclarations
>>>   1. produces 78 new errors
>>>
>>> NewLineAtEndOfFile
>>>   1. produces 111 new errors
>>>
>>> NoWhitespaceAfter
>>>   1. changing allowLineBreaks to false produces 183 new errors
>>>
>>> NoWhitespaceBefore
>>>   1. changing allowLineBreaks to false produces 1 new error
>>>
>>> OneStatementPerLine
>>>   1. produces 43 new errors
>>>
>>> ParameterNumber
>>>   1. removed (not present in proposed rules)
>>>
>>> ParenPad
>>>   1. produces 16666 new errors
>>>
>>> RedundantImport
>>>   1. produces 22 new errors
>>>
>>> RedundantModifier
>>>   1. adding INTERFACE_DEF (via default tokens) produces 3 new errors
>>>
>>> RegexpSingleLine
>>>   1. produces 46 new errors
>>>
>>> RightCurly
>>>   1. adding LITERAL_IF (via default tokens) produces 5 new errors
>>>
>>> UnusedImports
>>>   1. produces 80 new errors
>>>
>>> VisibilityModifier
>>>   1. left in, even though not present in proposed rules
>>>
>>> WhitespaceAfter
>>>   1. adding TYPECAST (via default tokens) produces 2203 new errors
>>>
>>> WhitespaceAround
>>>   1. left BAND, DIV, and STAR tokens, even though removed in proposed
>> rule
>>>
>>
> 

Reply via email to