Re: checkstyle-5.5 support, updated rules
On 03/03/12 00:57, Glenn Adams wrote: snip/ 2. for NoWhitespaceAfter, specified that line breaks are allowed after DOT; if this isn't done, then one cannot break a long line before *or* after DOT, and consequently could force long lines when not desired; this because the default settings for the NoWhitespaceBefore rule disallows a line break before DOT; DOT is not in NoWhitespaceBefore’s default list of tokens to check. Therefore it /is/ allowed to break a line before a dot. This already happens plenty of times in the codebase. Fixing the violations reported by the NoWhitespaceAfter rule is just a matter of moving the dot to the next line. Can we revert this change? G. Thanks, Vincent
Re: checkstyle-5.5 support, updated rules
On Mon, Mar 5, 2012 at 12:38 PM, Vincent Hennebert vhenneb...@gmail.comwrote: On 03/03/12 00:57, Glenn Adams wrote: snip/ 2. for NoWhitespaceAfter, specified that line breaks are allowed after DOT; if this isn't done, then one cannot break a long line before *or* after DOT, and consequently could force long lines when not desired; this because the default settings for the NoWhitespaceBefore rule disallows a line break before DOT; DOT is not in NoWhitespaceBefore’s default list of tokens to check. Therefore it /is/ allowed to break a line before a dot. This already happens plenty of times in the codebase. ok, i see I misread [1] and didn't notice that DOT was in the default tokens [1] http://checkstyle.sourceforge.net/config_whitespace.html#NoWhitespaceBefore Fixing the violations reported by the NoWhitespaceAfter rule is just a matter of moving the dot to the next line. Can we revert this change? actually this (allowing line break after DOT) was allowed in the checkstyle-5.1 rules that we have been using in FOP, so the new proposal to disallow line break after DOT is a new change (which hasn't yet been implemented); so it isn't really revering a change; in any case, i see that only 12 new errors are produced by disallowing line break after DOT, and, since i don't have a strong preference on this issue, i will go ahead and add the new restriction along with fixes for these 12 violations
Re: checkstyle-5.5 support, updated rules
On Mon, Mar 5, 2012 at 1:14 PM, Glenn Adams gl...@skynav.com wrote: On Mon, Mar 5, 2012 at 12:38 PM, Vincent Hennebert vhenneb...@gmail.comwrote: On 03/03/12 00:57, Glenn Adams wrote: snip/ 2. for NoWhitespaceAfter, specified that line breaks are allowed after DOT; if this isn't done, then one cannot break a long line before *or* after DOT, and consequently could force long lines when not desired; this because the default settings for the NoWhitespaceBefore rule disallows a line break before DOT; DOT is not in NoWhitespaceBefore’s default list of tokens to check. Therefore it /is/ allowed to break a line before a dot. This already happens plenty of times in the codebase. ok, i see I misread [1] and didn't notice that DOT was in the default tokens s/was in/was not in/ [1] http://checkstyle.sourceforge.net/config_whitespace.html#NoWhitespaceBefore
Re: checkstyle-5.5 support, updated rules
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=revisionrevision=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: ParenPad1 MethodParamPad 4316 WhitespaceAfter 2203 ExplicitInitialization 795 ImportOrder 255 NoWhitespaceAfter 183 NewLineAtEndOfFile 111 UnusedImports 80 MultipleVariableDeclarations78 RegexpSingleLine46 OneStatementPerLine 43 RedundantImport 22 DefaultComesLast9 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.
Re: checkstyle-5.5 support, updated rules
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 MultipleVariableDeclarations78 RegexpSingleLine46 OneStatementPerLine 43 RedundantImport 22 DefaultComesLast9 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.comwrote: 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=revisionrevision=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: ParenPad1 MethodParamPad 4316 WhitespaceAfter 2203 ExplicitInitialization 795 ImportOrder 255 NoWhitespaceAfter 183 NewLineAtEndOfFile 111 UnusedImports
Re: checkstyle-5.5 support, updated rules
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 MultipleVariableDeclarations78 RegexpSingleLine46 OneStatementPerLine 43 RedundantImport 22 DefaultComesLast9 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.comwrote: 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=revisionrevision=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
Re: checkstyle-5.5 support, updated rules
On Fri, Mar 2, 2012 at 11:18 AM, Vincent Hennebert vhenneb...@gmail.comwrote: I’d be happy to fix RegexpSingleLine and NewLineAtEndOfFile if that helps. Sure, that will help divide the effort. One way you might do this (which I will use), is: (1) changing the new checkstyle.* params in your local properties file to 5.5 config as I showed in my earlier email; (2) run checkstyle target to verify things are working (and zero errors); (3) uncommenting the specific rule you are making fixes for in the 5.5 config file; (4) rerunning checkstyle and making fixes until zero errors for that new rule; (5) committing those change; I'm sure I don't need to explain this to you, but it doesn't hurt to keep others informed.
Re: checkstyle-5.5 support, updated rules
On Fri, Mar 2, 2012 at 11:18 AM, Vincent Hennebert vhenneb...@gmail.comwrote: Will you also fix the tests? Eventually we want to check them as well. I agree that we will want to do so, but I'd prefer to handle the currently checkstyle tested code first, and then subsequently do a separate fix round to bring test code into compliance.
Re: checkstyle-5.5 support, updated rules
On Thu, Mar 1, 2012 at 5:30 PM, Glenn Adams gl...@skynav.com wrote: ParenPad1 MethodParamPad 4316 WhitespaceAfter 2203 ExplicitInitialization 795 Not implemented. I oppose enabling ParenPad or MethodParamPad for two reasons: (1) the large number of changes that would be required to fix violations; and (2) my preferred setting for styles differ from what was proposed. For similar reasons, I oppose adding TYPECAST to WhitespaceAfter. That leaves ExplicitInitialization, which I don't oppose, but will take some time to implement due to the large number of existing violations. NewLineAtEndOfFile 111 RegexpSingleLine46 Will wait for Vincent to implement these two. ImportOrder 255 NoWhitespaceAfter 183 NewLineAtEndOfFile 111 UnusedImports 80 MultipleVariableDeclarations78 RegexpSingleLine46 OneStatementPerLine 43 RedundantImport 22 DefaultComesLast9 RightCurly 5 RedundantModifier 3 GenericWhitespace 1 NoWhitespaceBefore 1 These are now implemented (with the checkstyle-5.5 config), but with the following minor changes from what Vincent had proposed: 1. for ImportOrder, specified that static imports are under, not top; this corresponds to the practice that already existed in FOP; 2. for NoWhitespaceAfter, specified that line breaks are allowed after DOT; if this isn't done, then one cannot break a long line before *or* after DOT, and consequently could force long lines when not desired; this because the default settings for the NoWhitespaceBefore rule disallows a line break before DOT; G.
checkstyle-5.5 support, updated rules
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=revisionrevision=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: ParenPad1 MethodParamPad 4316 WhitespaceAfter 2203 ExplicitInitialization 795 ImportOrder 255 NoWhitespaceAfter 183 NewLineAtEndOfFile 111 UnusedImports 80 MultipleVariableDeclarations78 RegexpSingleLine46 OneStatementPerLine 43 RedundantImport 22 DefaultComesLast9 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 1 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