You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ofbiz.apache.org by Gil Portenseigne <gi...@nereide.fr> on 2022/07/04 15:24:43 UTC
Codenarc integration, rules to use.
Hello Devs,
I would like to continue Codenarc integration in OFBiz (OFBIZ-11167).
For those who are not aware, Codenarc is an analysis tools for Groovy
for defects, bad practices, inconsistencies, style issues and more.
For this purpose we need to agree about the ruleset to put in place.
I took as a basis the ruleset :
https://codenarc.org/StarterRuleSet-AllRulesByCategory.groovy.txt
And started to fix some in https://github.com/apache/ofbiz-framework/pull/517
Before doing the complete work, a first rule made me wonder if that is a
choice that we will be doing as a community :
IfStatementBraces - Use braces for if statements, even for a single statement.
There are 234 occurrences in the project, and I guess that some opinions
might diverge on this subject.
Moreover, some rules needs lots of fixes in the project (those with more
than 200 occurrences) :
+------------+---------------------------------------------------------------------+
| Number of | Rule name and details |
| occurrence | |
+============+=====================================================================+
| 9883 | UnnecessaryGString - String objects should be created with single |
| | quotes, and GString objects created with double quotes. Creating |
| | normal String objects with double quotes is confusing to readers. |
+------------+---------------------------------------------------------------------+
| 4569 | DuplicateStringLiteral - Code containing duplicate String literals |
| | can usually be improved by declaring the String as a constant |
| | field. The ignoreStrings property () can optionally specify a |
| | comma-separated list of Strings to ignore. |
+------------+---------------------------------------------------------------------+
| 4209 | SpaceAroundMapEntryColon - Check for configured formatting of |
| | whitespace around colons for literal Map entries. |
| | The characterBeforeColonRegex and characterAfterColonRegex |
| | properties specify a regular expression that must match the |
| | character before/after the colon. |
+------------+---------------------------------------------------------------------+
| 1448 | LineLength - Checks the maximum length for each line of |
| | source code. It checks for number of characters, so lines |
| | that include tabs may appear longer than the allowed number |
| | when viewing the file. The maximum line length can be |
| | configured by setting the length property, which defaults to 120. |
+------------+---------------------------------------------------------------------+
| 885 | UnnecessaryGetter - Checks for explicit calls to getter/accessor |
| | methods which can, for the most part, be replaced by property |
| | access. A getter is defined as a method call that matches |
| | get[A-Z] but not getClass() or get[A-Z][A-Z] such as getURL(). |
| | Getters do not take method arguments. The ignoreMethodNames |
| | property (null) can specify method names that should be ignored |
| | , optionally containing wildcard characters ('*' or '?'). |
+------------+---------------------------------------------------------------------+
| 601 | NoDef - def should not be used. You should replace it with |
| | concrete type. |
+------------+---------------------------------------------------------------------+
| 485 | MethodReturnTypeRequired - Checks that method return types |
| | are not dynamic, that is they are explicitly stated and |
| | different than def. |
+------------+---------------------------------------------------------------------+
| 484 | Indentation - Check indentation for class and method |
| | declarations, and initial statements. |
+------------+---------------------------------------------------------------------+
| 482 | UnnecessaryReturnKeyword - In Groovy, the return keyword |
| | is often optional. If a statement is the last line in a |
| | method or closure then you do not need to have the return keyword. |
+------------+---------------------------------------------------------------------+
| 407 | UnnecessaryObjectReferences - Violations are triggered when |
| | an excessive set of consecutive statements all reference |
| | the same variable. This can be made more readable by using |
| | a with or identity block. |
+------------+---------------------------------------------------------------------+
| 345 | CompileStatic - Check that classes are explicitely annotated |
| | with either @GrailsCompileStatic, @CompileStatic or @CompileDynamic |
+------------+---------------------------------------------------------------------+
| 320 | ExplicitCallToEqualsMethod - This rule detects when the |
| | equals(Object) method is called directly in code instead |
| | of using the == or != operator. A groovier way to |
| | express this: a.equals(b) is this: a == b and a groovier |
| | way to express : !a.equals(b) is : a != b |
+------------+---------------------------------------------------------------------+
| 235 | IfStatementBraces - Use braces for if statements, |
| | even for a single statement. |
+------------+---------------------------------------------------------------------+
| 235 | SpaceAroundOperator - Check that there is at least one space |
| | (blank) or whitespace around each binary operator. |
+------------+---------------------------------------------------------------------+
| 211 | NestedBlockDepth - Checks for blocks or closures nested more |
| | than maxNestedBlockDepth (5) levels deep. |
+------------+---------------------------------------------------------------------+
| 201 | TrailingWhitespace - Checks that no lines of source code |
| | end with whitespace characters. |
+------------+---------------------------------------------------------------------+
I think that if someone want to express an opposition about applying one
of the above rule, i would be great to express it and discuss it here.
My opinion is that it could be nice to adopt every rules, to respect
standard best practice. Even if it implies lot of code changes.
Thanks in advance for your feedback.
Regards,
Gil
Re: Codenarc integration, rules to use.
Posted by Gil Portenseigne <gi...@nereide.fr>.
Forgot the ref https://github.com/CodeNarc/CodeNarc/issues/331
Le 12 juillet 2022 21:11:13 GMT+02:00, Gil Portenseigne <gi...@nereide.fr> a écrit :
>Hello,
>
>I agree with you, i find out here [1] that it is customisable.
>
>So we can agree upon this variation of the rule !
>
>I have not tested yet, will see in the near future
>
>Thanks
>
>Gil
>
>Le 11 juillet 2022 17:57:45 GMT+02:00, Scott Gray <le...@gmail.com> a écrit :
>>+1 in general from me although some of those rules might be challenging to
>>resolve. For example DuplicateStringLiteral could be referring to an
>>entity name being used in delegator queries more than once, I don't think
>>we'd prefer to see that extracted to a constant.
>>
>>SpaceAroundMapEntryColon I'm not so sure about, my preference for
>>legibility has always been [someKey: someValue]. I find
>>[someKey:someValue] a bit too condensed and harder to differentiate key
>>from value.
>>
>>Regards
>>Scott
>>
>>On Mon, 4 Jul 2022 at 16:25, Gil Portenseigne <gi...@nereide.fr>
>>wrote:
>>
>>> Hello Devs,
>>>
>>> I would like to continue Codenarc integration in OFBiz (OFBIZ-11167).
>>>
>>> For those who are not aware, Codenarc is an analysis tools for Groovy
>>> for defects, bad practices, inconsistencies, style issues and more.
>>>
>>> For this purpose we need to agree about the ruleset to put in place.
>>>
>>> I took as a basis the ruleset :
>>> https://codenarc.org/StarterRuleSet-AllRulesByCategory.groovy.txt
>>>
>>> And started to fix some in
>>> https://github.com/apache/ofbiz-framework/pull/517
>>>
>>> Before doing the complete work, a first rule made me wonder if that is a
>>> choice that we will be doing as a community :
>>>
>>> IfStatementBraces - Use braces for if statements, even for a single
>>> statement.
>>>
>>> There are 234 occurrences in the project, and I guess that some opinions
>>> might diverge on this subject.
>>>
>>> Moreover, some rules needs lots of fixes in the project (those with more
>>> than 200 occurrences) :
>>>
>>>
>>> +------------+---------------------------------------------------------------------+
>>> | Number of | Rule name and details
>>> |
>>> | occurrence |
>>> |
>>>
>>> +============+=====================================================================+
>>> | 9883 | UnnecessaryGString - String objects should be created with
>>> single |
>>> | | quotes, and GString objects created with double quotes.
>>> Creating |
>>> | | normal String objects with double quotes is confusing to
>>> readers. |
>>>
>>> +------------+---------------------------------------------------------------------+
>>> | 4569 | DuplicateStringLiteral - Code containing duplicate String
>>> literals |
>>> | | can usually be improved by declaring the String as a
>>> constant |
>>> | | field. The ignoreStrings property () can optionally
>>> specify a |
>>> | | comma-separated list of Strings to ignore.
>>> |
>>>
>>> +------------+---------------------------------------------------------------------+
>>> | 4209 | SpaceAroundMapEntryColon - Check for configured formatting
>>> of |
>>> | | whitespace around colons for literal Map entries.
>>> |
>>> | | The characterBeforeColonRegex and
>>> characterAfterColonRegex |
>>> | | properties specify a regular expression that must match
>>> the |
>>> | | character before/after the colon.
>>> |
>>>
>>> +------------+---------------------------------------------------------------------+
>>> | 1448 | LineLength - Checks the maximum length for each line of
>>> |
>>> | | source code. It checks for number of characters, so lines
>>> |
>>> | | that include tabs may appear longer than the allowed
>>> number |
>>> | | when viewing the file. The maximum line length can be
>>> |
>>> | | configured by setting the length property, which defaults
>>> to 120. |
>>>
>>> +------------+---------------------------------------------------------------------+
>>> | 885 | UnnecessaryGetter - Checks for explicit calls to
>>> getter/accessor |
>>> | | methods which can, for the most part, be replaced by
>>> property |
>>> | | access. A getter is defined as a method call that
>>> matches |
>>> | | get[A-Z] but not getClass() or get[A-Z][A-Z] such as
>>> getURL(). |
>>> | | Getters do not take method arguments. The
>>> ignoreMethodNames |
>>> | | property (null) can specify method names that should be
>>> ignored |
>>> | | , optionally containing wildcard characters ('*' or '?').
>>> |
>>>
>>> +------------+---------------------------------------------------------------------+
>>> | 601 | NoDef - def should not be used. You should replace it with
>>> |
>>> | | concrete type.
>>> |
>>>
>>> +------------+---------------------------------------------------------------------+
>>> | 485 | MethodReturnTypeRequired - Checks that method return
>>> types |
>>> | | are not dynamic, that is they are explicitly stated and
>>> |
>>> | | different than def.
>>> |
>>>
>>> +------------+---------------------------------------------------------------------+
>>> | 484 | Indentation - Check indentation for class and method
>>> |
>>> | | declarations, and initial statements.
>>> |
>>>
>>> +------------+---------------------------------------------------------------------+
>>> | 482 | UnnecessaryReturnKeyword - In Groovy, the return keyword
>>> |
>>> | | is often optional. If a statement is the last line in a
>>> |
>>> | | method or closure then you do not need to have the return
>>> keyword. |
>>>
>>> +------------+---------------------------------------------------------------------+
>>> | 407 | UnnecessaryObjectReferences - Violations are triggered
>>> when |
>>> | | an excessive set of consecutive statements all reference
>>> |
>>> | | the same variable. This can be made more readable by
>>> using |
>>> | | a with or identity block.
>>> |
>>>
>>> +------------+---------------------------------------------------------------------+
>>> | 345 | CompileStatic - Check that classes are explicitely
>>> annotated |
>>> | | with either @GrailsCompileStatic, @CompileStatic or
>>> @CompileDynamic |
>>>
>>> +------------+---------------------------------------------------------------------+
>>> | 320 | ExplicitCallToEqualsMethod - This rule detects when the
>>> |
>>> | | equals(Object) method is called directly in code instead
>>> |
>>> | | of using the == or != operator. A groovier way to
>>> |
>>> | | express this: a.equals(b) is this: a == b and a groovier
>>> |
>>> | | way to express : !a.equals(b) is : a != b
>>> |
>>>
>>> +------------+---------------------------------------------------------------------+
>>> | 235 | IfStatementBraces - Use braces for if statements,
>>> |
>>> | | even for a single statement.
>>> |
>>>
>>> +------------+---------------------------------------------------------------------+
>>> | 235 | SpaceAroundOperator - Check that there is at least one
>>> space |
>>> | | (blank) or whitespace around each binary operator.
>>> |
>>>
>>> +------------+---------------------------------------------------------------------+
>>> | 211 | NestedBlockDepth - Checks for blocks or closures nested
>>> more |
>>> | | than maxNestedBlockDepth (5) levels deep.
>>> |
>>>
>>> +------------+---------------------------------------------------------------------+
>>> | 201 | TrailingWhitespace - Checks that no lines of source code
>>> |
>>> | | end with whitespace characters.
>>> |
>>>
>>> +------------+---------------------------------------------------------------------+
>>>
>>> I think that if someone want to express an opposition about applying one
>>> of the above rule, i would be great to express it and discuss it here.
>>>
>>> My opinion is that it could be nice to adopt every rules, to respect
>>> standard best practice. Even if it implies lot of code changes.
>>>
>>> Thanks in advance for your feedback.
>>>
>>> Regards,
>>>
>>> Gil
>>>
>
>--
>Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser ma brièveté.
--
Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser ma brièveté.
Re: Codenarc integration, rules to use.
Posted by Gil Portenseigne <gi...@nereide.fr>.
Hello,
I agree with you, i find out here [1] that it is customisable.
So we can agree upon this variation of the rule !
I have not tested yet, will see in the near future
Thanks
Gil
Le 11 juillet 2022 17:57:45 GMT+02:00, Scott Gray <le...@gmail.com> a écrit :
>+1 in general from me although some of those rules might be challenging to
>resolve. For example DuplicateStringLiteral could be referring to an
>entity name being used in delegator queries more than once, I don't think
>we'd prefer to see that extracted to a constant.
>
>SpaceAroundMapEntryColon I'm not so sure about, my preference for
>legibility has always been [someKey: someValue]. I find
>[someKey:someValue] a bit too condensed and harder to differentiate key
>from value.
>
>Regards
>Scott
>
>On Mon, 4 Jul 2022 at 16:25, Gil Portenseigne <gi...@nereide.fr>
>wrote:
>
>> Hello Devs,
>>
>> I would like to continue Codenarc integration in OFBiz (OFBIZ-11167).
>>
>> For those who are not aware, Codenarc is an analysis tools for Groovy
>> for defects, bad practices, inconsistencies, style issues and more.
>>
>> For this purpose we need to agree about the ruleset to put in place.
>>
>> I took as a basis the ruleset :
>> https://codenarc.org/StarterRuleSet-AllRulesByCategory.groovy.txt
>>
>> And started to fix some in
>> https://github.com/apache/ofbiz-framework/pull/517
>>
>> Before doing the complete work, a first rule made me wonder if that is a
>> choice that we will be doing as a community :
>>
>> IfStatementBraces - Use braces for if statements, even for a single
>> statement.
>>
>> There are 234 occurrences in the project, and I guess that some opinions
>> might diverge on this subject.
>>
>> Moreover, some rules needs lots of fixes in the project (those with more
>> than 200 occurrences) :
>>
>>
>> +------------+---------------------------------------------------------------------+
>> | Number of | Rule name and details
>> |
>> | occurrence |
>> |
>>
>> +============+=====================================================================+
>> | 9883 | UnnecessaryGString - String objects should be created with
>> single |
>> | | quotes, and GString objects created with double quotes.
>> Creating |
>> | | normal String objects with double quotes is confusing to
>> readers. |
>>
>> +------------+---------------------------------------------------------------------+
>> | 4569 | DuplicateStringLiteral - Code containing duplicate String
>> literals |
>> | | can usually be improved by declaring the String as a
>> constant |
>> | | field. The ignoreStrings property () can optionally
>> specify a |
>> | | comma-separated list of Strings to ignore.
>> |
>>
>> +------------+---------------------------------------------------------------------+
>> | 4209 | SpaceAroundMapEntryColon - Check for configured formatting
>> of |
>> | | whitespace around colons for literal Map entries.
>> |
>> | | The characterBeforeColonRegex and
>> characterAfterColonRegex |
>> | | properties specify a regular expression that must match
>> the |
>> | | character before/after the colon.
>> |
>>
>> +------------+---------------------------------------------------------------------+
>> | 1448 | LineLength - Checks the maximum length for each line of
>> |
>> | | source code. It checks for number of characters, so lines
>> |
>> | | that include tabs may appear longer than the allowed
>> number |
>> | | when viewing the file. The maximum line length can be
>> |
>> | | configured by setting the length property, which defaults
>> to 120. |
>>
>> +------------+---------------------------------------------------------------------+
>> | 885 | UnnecessaryGetter - Checks for explicit calls to
>> getter/accessor |
>> | | methods which can, for the most part, be replaced by
>> property |
>> | | access. A getter is defined as a method call that
>> matches |
>> | | get[A-Z] but not getClass() or get[A-Z][A-Z] such as
>> getURL(). |
>> | | Getters do not take method arguments. The
>> ignoreMethodNames |
>> | | property (null) can specify method names that should be
>> ignored |
>> | | , optionally containing wildcard characters ('*' or '?').
>> |
>>
>> +------------+---------------------------------------------------------------------+
>> | 601 | NoDef - def should not be used. You should replace it with
>> |
>> | | concrete type.
>> |
>>
>> +------------+---------------------------------------------------------------------+
>> | 485 | MethodReturnTypeRequired - Checks that method return
>> types |
>> | | are not dynamic, that is they are explicitly stated and
>> |
>> | | different than def.
>> |
>>
>> +------------+---------------------------------------------------------------------+
>> | 484 | Indentation - Check indentation for class and method
>> |
>> | | declarations, and initial statements.
>> |
>>
>> +------------+---------------------------------------------------------------------+
>> | 482 | UnnecessaryReturnKeyword - In Groovy, the return keyword
>> |
>> | | is often optional. If a statement is the last line in a
>> |
>> | | method or closure then you do not need to have the return
>> keyword. |
>>
>> +------------+---------------------------------------------------------------------+
>> | 407 | UnnecessaryObjectReferences - Violations are triggered
>> when |
>> | | an excessive set of consecutive statements all reference
>> |
>> | | the same variable. This can be made more readable by
>> using |
>> | | a with or identity block.
>> |
>>
>> +------------+---------------------------------------------------------------------+
>> | 345 | CompileStatic - Check that classes are explicitely
>> annotated |
>> | | with either @GrailsCompileStatic, @CompileStatic or
>> @CompileDynamic |
>>
>> +------------+---------------------------------------------------------------------+
>> | 320 | ExplicitCallToEqualsMethod - This rule detects when the
>> |
>> | | equals(Object) method is called directly in code instead
>> |
>> | | of using the == or != operator. A groovier way to
>> |
>> | | express this: a.equals(b) is this: a == b and a groovier
>> |
>> | | way to express : !a.equals(b) is : a != b
>> |
>>
>> +------------+---------------------------------------------------------------------+
>> | 235 | IfStatementBraces - Use braces for if statements,
>> |
>> | | even for a single statement.
>> |
>>
>> +------------+---------------------------------------------------------------------+
>> | 235 | SpaceAroundOperator - Check that there is at least one
>> space |
>> | | (blank) or whitespace around each binary operator.
>> |
>>
>> +------------+---------------------------------------------------------------------+
>> | 211 | NestedBlockDepth - Checks for blocks or closures nested
>> more |
>> | | than maxNestedBlockDepth (5) levels deep.
>> |
>>
>> +------------+---------------------------------------------------------------------+
>> | 201 | TrailingWhitespace - Checks that no lines of source code
>> |
>> | | end with whitespace characters.
>> |
>>
>> +------------+---------------------------------------------------------------------+
>>
>> I think that if someone want to express an opposition about applying one
>> of the above rule, i would be great to express it and discuss it here.
>>
>> My opinion is that it could be nice to adopt every rules, to respect
>> standard best practice. Even if it implies lot of code changes.
>>
>> Thanks in advance for your feedback.
>>
>> Regards,
>>
>> Gil
>>
--
Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser ma brièveté.
Re: Codenarc integration, rules to use.
Posted by Scott Gray <le...@gmail.com>.
+1 in general from me although some of those rules might be challenging to
resolve. For example DuplicateStringLiteral could be referring to an
entity name being used in delegator queries more than once, I don't think
we'd prefer to see that extracted to a constant.
SpaceAroundMapEntryColon I'm not so sure about, my preference for
legibility has always been [someKey: someValue]. I find
[someKey:someValue] a bit too condensed and harder to differentiate key
from value.
Regards
Scott
On Mon, 4 Jul 2022 at 16:25, Gil Portenseigne <gi...@nereide.fr>
wrote:
> Hello Devs,
>
> I would like to continue Codenarc integration in OFBiz (OFBIZ-11167).
>
> For those who are not aware, Codenarc is an analysis tools for Groovy
> for defects, bad practices, inconsistencies, style issues and more.
>
> For this purpose we need to agree about the ruleset to put in place.
>
> I took as a basis the ruleset :
> https://codenarc.org/StarterRuleSet-AllRulesByCategory.groovy.txt
>
> And started to fix some in
> https://github.com/apache/ofbiz-framework/pull/517
>
> Before doing the complete work, a first rule made me wonder if that is a
> choice that we will be doing as a community :
>
> IfStatementBraces - Use braces for if statements, even for a single
> statement.
>
> There are 234 occurrences in the project, and I guess that some opinions
> might diverge on this subject.
>
> Moreover, some rules needs lots of fixes in the project (those with more
> than 200 occurrences) :
>
>
> +------------+---------------------------------------------------------------------+
> | Number of | Rule name and details
> |
> | occurrence |
> |
>
> +============+=====================================================================+
> | 9883 | UnnecessaryGString - String objects should be created with
> single |
> | | quotes, and GString objects created with double quotes.
> Creating |
> | | normal String objects with double quotes is confusing to
> readers. |
>
> +------------+---------------------------------------------------------------------+
> | 4569 | DuplicateStringLiteral - Code containing duplicate String
> literals |
> | | can usually be improved by declaring the String as a
> constant |
> | | field. The ignoreStrings property () can optionally
> specify a |
> | | comma-separated list of Strings to ignore.
> |
>
> +------------+---------------------------------------------------------------------+
> | 4209 | SpaceAroundMapEntryColon - Check for configured formatting
> of |
> | | whitespace around colons for literal Map entries.
> |
> | | The characterBeforeColonRegex and
> characterAfterColonRegex |
> | | properties specify a regular expression that must match
> the |
> | | character before/after the colon.
> |
>
> +------------+---------------------------------------------------------------------+
> | 1448 | LineLength - Checks the maximum length for each line of
> |
> | | source code. It checks for number of characters, so lines
> |
> | | that include tabs may appear longer than the allowed
> number |
> | | when viewing the file. The maximum line length can be
> |
> | | configured by setting the length property, which defaults
> to 120. |
>
> +------------+---------------------------------------------------------------------+
> | 885 | UnnecessaryGetter - Checks for explicit calls to
> getter/accessor |
> | | methods which can, for the most part, be replaced by
> property |
> | | access. A getter is defined as a method call that
> matches |
> | | get[A-Z] but not getClass() or get[A-Z][A-Z] such as
> getURL(). |
> | | Getters do not take method arguments. The
> ignoreMethodNames |
> | | property (null) can specify method names that should be
> ignored |
> | | , optionally containing wildcard characters ('*' or '?').
> |
>
> +------------+---------------------------------------------------------------------+
> | 601 | NoDef - def should not be used. You should replace it with
> |
> | | concrete type.
> |
>
> +------------+---------------------------------------------------------------------+
> | 485 | MethodReturnTypeRequired - Checks that method return
> types |
> | | are not dynamic, that is they are explicitly stated and
> |
> | | different than def.
> |
>
> +------------+---------------------------------------------------------------------+
> | 484 | Indentation - Check indentation for class and method
> |
> | | declarations, and initial statements.
> |
>
> +------------+---------------------------------------------------------------------+
> | 482 | UnnecessaryReturnKeyword - In Groovy, the return keyword
> |
> | | is often optional. If a statement is the last line in a
> |
> | | method or closure then you do not need to have the return
> keyword. |
>
> +------------+---------------------------------------------------------------------+
> | 407 | UnnecessaryObjectReferences - Violations are triggered
> when |
> | | an excessive set of consecutive statements all reference
> |
> | | the same variable. This can be made more readable by
> using |
> | | a with or identity block.
> |
>
> +------------+---------------------------------------------------------------------+
> | 345 | CompileStatic - Check that classes are explicitely
> annotated |
> | | with either @GrailsCompileStatic, @CompileStatic or
> @CompileDynamic |
>
> +------------+---------------------------------------------------------------------+
> | 320 | ExplicitCallToEqualsMethod - This rule detects when the
> |
> | | equals(Object) method is called directly in code instead
> |
> | | of using the == or != operator. A groovier way to
> |
> | | express this: a.equals(b) is this: a == b and a groovier
> |
> | | way to express : !a.equals(b) is : a != b
> |
>
> +------------+---------------------------------------------------------------------+
> | 235 | IfStatementBraces - Use braces for if statements,
> |
> | | even for a single statement.
> |
>
> +------------+---------------------------------------------------------------------+
> | 235 | SpaceAroundOperator - Check that there is at least one
> space |
> | | (blank) or whitespace around each binary operator.
> |
>
> +------------+---------------------------------------------------------------------+
> | 211 | NestedBlockDepth - Checks for blocks or closures nested
> more |
> | | than maxNestedBlockDepth (5) levels deep.
> |
>
> +------------+---------------------------------------------------------------------+
> | 201 | TrailingWhitespace - Checks that no lines of source code
> |
> | | end with whitespace characters.
> |
>
> +------------+---------------------------------------------------------------------+
>
> I think that if someone want to express an opposition about applying one
> of the above rule, i would be great to express it and discuss it here.
>
> My opinion is that it could be nice to adopt every rules, to respect
> standard best practice. Even if it implies lot of code changes.
>
> Thanks in advance for your feedback.
>
> Regards,
>
> Gil
>
Re: Codenarc integration, rules to use.
Posted by Jacques Le Roux <ja...@les7arts.com>.
Hi Gil,
Great initiative, so far it seems we have a lazy consensus
We (almost*) did it for Java, why not for Groovy :)
Jacques
* OFBIZ-12341
Le 04/07/2022 à 17:24, Gil Portenseigne a écrit :
> Hello Devs,
>
> I would like to continue Codenarc integration in OFBiz (OFBIZ-11167).
>
> For those who are not aware, Codenarc is an analysis tools for Groovy
> for defects, bad practices, inconsistencies, style issues and more.
>
> For this purpose we need to agree about the ruleset to put in place.
>
> I took as a basis the ruleset :
> https://codenarc.org/StarterRuleSet-AllRulesByCategory.groovy.txt
>
> And started to fix some in https://github.com/apache/ofbiz-framework/pull/517
>
> Before doing the complete work, a first rule made me wonder if that is a
> choice that we will be doing as a community :
>
> IfStatementBraces - Use braces for if statements, even for a single statement.
>
> There are 234 occurrences in the project, and I guess that some opinions
> might diverge on this subject.
>
> Moreover, some rules needs lots of fixes in the project (those with more
> than 200 occurrences) :
>
> +------------+---------------------------------------------------------------------+
> | Number of | Rule name and details |
> | occurrence | |
> +============+=====================================================================+
> | 9883 | UnnecessaryGString - String objects should be created with single |
> | | quotes, and GString objects created with double quotes. Creating |
> | | normal String objects with double quotes is confusing to readers. |
> +------------+---------------------------------------------------------------------+
> | 4569 | DuplicateStringLiteral - Code containing duplicate String literals |
> | | can usually be improved by declaring the String as a constant |
> | | field. The ignoreStrings property () can optionally specify a |
> | | comma-separated list of Strings to ignore. |
> +------------+---------------------------------------------------------------------+
> | 4209 | SpaceAroundMapEntryColon - Check for configured formatting of |
> | | whitespace around colons for literal Map entries. |
> | | The characterBeforeColonRegex and characterAfterColonRegex |
> | | properties specify a regular expression that must match the |
> | | character before/after the colon. |
> +------------+---------------------------------------------------------------------+
> | 1448 | LineLength - Checks the maximum length for each line of |
> | | source code. It checks for number of characters, so lines |
> | | that include tabs may appear longer than the allowed number |
> | | when viewing the file. The maximum line length can be |
> | | configured by setting the length property, which defaults to 120. |
> +------------+---------------------------------------------------------------------+
> | 885 | UnnecessaryGetter - Checks for explicit calls to getter/accessor |
> | | methods which can, for the most part, be replaced by property |
> | | access. A getter is defined as a method call that matches |
> | | get[A-Z] but not getClass() or get[A-Z][A-Z] such as getURL(). |
> | | Getters do not take method arguments. The ignoreMethodNames |
> | | property (null) can specify method names that should be ignored |
> | | , optionally containing wildcard characters ('*' or '?'). |
> +------------+---------------------------------------------------------------------+
> | 601 | NoDef - def should not be used. You should replace it with |
> | | concrete type. |
> +------------+---------------------------------------------------------------------+
> | 485 | MethodReturnTypeRequired - Checks that method return types |
> | | are not dynamic, that is they are explicitly stated and |
> | | different than def. |
> +------------+---------------------------------------------------------------------+
> | 484 | Indentation - Check indentation for class and method |
> | | declarations, and initial statements. |
> +------------+---------------------------------------------------------------------+
> | 482 | UnnecessaryReturnKeyword - In Groovy, the return keyword |
> | | is often optional. If a statement is the last line in a |
> | | method or closure then you do not need to have the return keyword. |
> +------------+---------------------------------------------------------------------+
> | 407 | UnnecessaryObjectReferences - Violations are triggered when |
> | | an excessive set of consecutive statements all reference |
> | | the same variable. This can be made more readable by using |
> | | a with or identity block. |
> +------------+---------------------------------------------------------------------+
> | 345 | CompileStatic - Check that classes are explicitely annotated |
> | | with either @GrailsCompileStatic, @CompileStatic or @CompileDynamic |
> +------------+---------------------------------------------------------------------+
> | 320 | ExplicitCallToEqualsMethod - This rule detects when the |
> | | equals(Object) method is called directly in code instead |
> | | of using the == or != operator. A groovier way to |
> | | express this: a.equals(b) is this: a == b and a groovier |
> | | way to express : !a.equals(b) is : a != b |
> +------------+---------------------------------------------------------------------+
> | 235 | IfStatementBraces - Use braces for if statements, |
> | | even for a single statement. |
> +------------+---------------------------------------------------------------------+
> | 235 | SpaceAroundOperator - Check that there is at least one space |
> | | (blank) or whitespace around each binary operator. |
> +------------+---------------------------------------------------------------------+
> | 211 | NestedBlockDepth - Checks for blocks or closures nested more |
> | | than maxNestedBlockDepth (5) levels deep. |
> +------------+---------------------------------------------------------------------+
> | 201 | TrailingWhitespace - Checks that no lines of source code |
> | | end with whitespace characters. |
> +------------+---------------------------------------------------------------------+
>
> I think that if someone want to express an opposition about applying one
> of the above rule, i would be great to express it and discuss it here.
>
> My opinion is that it could be nice to adopt every rules, to respect
> standard best practice. Even if it implies lot of code changes.
>
> Thanks in advance for your feedback.
>
> Regards,
>
> Gil
Re: Codenarc integration, rules to use.
Posted by Ashish Vijaywargiya <as...@hotwaxsystems.com>.
+1
This can be a great addition to the project. Thank you, Gil.
--
Kind Regards,
Ashish Vijaywargiya
Vice President of Operations
*HotWax Systems*
*Enterprise open source experts*
http://www.hotwaxsystems.com
On Mon, Jul 4, 2022 at 8:54 PM Gil Portenseigne <gi...@nereide.fr>
wrote:
> Hello Devs,
>
> I would like to continue Codenarc integration in OFBiz (OFBIZ-11167).
>
> For those who are not aware, Codenarc is an analysis tools for Groovy
> for defects, bad practices, inconsistencies, style issues and more.
>
> For this purpose we need to agree about the ruleset to put in place.
>
> I took as a basis the ruleset :
> https://codenarc.org/StarterRuleSet-AllRulesByCategory.groovy.txt
>
> And started to fix some in
> https://github.com/apache/ofbiz-framework/pull/517
>
> Before doing the complete work, a first rule made me wonder if that is a
> choice that we will be doing as a community :
>
> IfStatementBraces - Use braces for if statements, even for a single
> statement.
>
> There are 234 occurrences in the project, and I guess that some opinions
> might diverge on this subject.
>
> Moreover, some rules needs lots of fixes in the project (those with more
> than 200 occurrences) :
>
>
> +------------+---------------------------------------------------------------------+
> | Number of | Rule name and details
> |
> | occurrence |
> |
>
> +============+=====================================================================+
> | 9883 | UnnecessaryGString - String objects should be created with
> single |
> | | quotes, and GString objects created with double quotes.
> Creating |
> | | normal String objects with double quotes is confusing to
> readers. |
>
> +------------+---------------------------------------------------------------------+
> | 4569 | DuplicateStringLiteral - Code containing duplicate String
> literals |
> | | can usually be improved by declaring the String as a
> constant |
> | | field. The ignoreStrings property () can optionally
> specify a |
> | | comma-separated list of Strings to ignore.
> |
>
> +------------+---------------------------------------------------------------------+
> | 4209 | SpaceAroundMapEntryColon - Check for configured formatting
> of |
> | | whitespace around colons for literal Map entries.
> |
> | | The characterBeforeColonRegex and
> characterAfterColonRegex |
> | | properties specify a regular expression that must match
> the |
> | | character before/after the colon.
> |
>
> +------------+---------------------------------------------------------------------+
> | 1448 | LineLength - Checks the maximum length for each line of
> |
> | | source code. It checks for number of characters, so lines
> |
> | | that include tabs may appear longer than the allowed
> number |
> | | when viewing the file. The maximum line length can be
> |
> | | configured by setting the length property, which defaults
> to 120. |
>
> +------------+---------------------------------------------------------------------+
> | 885 | UnnecessaryGetter - Checks for explicit calls to
> getter/accessor |
> | | methods which can, for the most part, be replaced by
> property |
> | | access. A getter is defined as a method call that
> matches |
> | | get[A-Z] but not getClass() or get[A-Z][A-Z] such as
> getURL(). |
> | | Getters do not take method arguments. The
> ignoreMethodNames |
> | | property (null) can specify method names that should be
> ignored |
> | | , optionally containing wildcard characters ('*' or '?').
> |
>
> +------------+---------------------------------------------------------------------+
> | 601 | NoDef - def should not be used. You should replace it with
> |
> | | concrete type.
> |
>
> +------------+---------------------------------------------------------------------+
> | 485 | MethodReturnTypeRequired - Checks that method return
> types |
> | | are not dynamic, that is they are explicitly stated and
> |
> | | different than def.
> |
>
> +------------+---------------------------------------------------------------------+
> | 484 | Indentation - Check indentation for class and method
> |
> | | declarations, and initial statements.
> |
>
> +------------+---------------------------------------------------------------------+
> | 482 | UnnecessaryReturnKeyword - In Groovy, the return keyword
> |
> | | is often optional. If a statement is the last line in a
> |
> | | method or closure then you do not need to have the return
> keyword. |
>
> +------------+---------------------------------------------------------------------+
> | 407 | UnnecessaryObjectReferences - Violations are triggered
> when |
> | | an excessive set of consecutive statements all reference
> |
> | | the same variable. This can be made more readable by
> using |
> | | a with or identity block.
> |
>
> +------------+---------------------------------------------------------------------+
> | 345 | CompileStatic - Check that classes are explicitely
> annotated |
> | | with either @GrailsCompileStatic, @CompileStatic or
> @CompileDynamic |
>
> +------------+---------------------------------------------------------------------+
> | 320 | ExplicitCallToEqualsMethod - This rule detects when the
> |
> | | equals(Object) method is called directly in code instead
> |
> | | of using the == or != operator. A groovier way to
> |
> | | express this: a.equals(b) is this: a == b and a groovier
> |
> | | way to express : !a.equals(b) is : a != b
> |
>
> +------------+---------------------------------------------------------------------+
> | 235 | IfStatementBraces - Use braces for if statements,
> |
> | | even for a single statement.
> |
>
> +------------+---------------------------------------------------------------------+
> | 235 | SpaceAroundOperator - Check that there is at least one
> space |
> | | (blank) or whitespace around each binary operator.
> |
>
> +------------+---------------------------------------------------------------------+
> | 211 | NestedBlockDepth - Checks for blocks or closures nested
> more |
> | | than maxNestedBlockDepth (5) levels deep.
> |
>
> +------------+---------------------------------------------------------------------+
> | 201 | TrailingWhitespace - Checks that no lines of source code
> |
> | | end with whitespace characters.
> |
>
> +------------+---------------------------------------------------------------------+
>
> I think that if someone want to express an opposition about applying one
> of the above rule, i would be great to express it and discuss it here.
>
> My opinion is that it could be nice to adopt every rules, to respect
> standard best practice. Even if it implies lot of code changes.
>
> Thanks in advance for your feedback.
>
> Regards,
>
> Gil
>
Re: Codenarc integration, rules to use.
Posted by Gil Portenseigne <gi...@nereide.fr>.
Hello Jacques,
Thanks for your feedback, I'll go with that.
Gil
On Mon, Jul 25, 2022 at 10:08:20AM +0200, Jacques Le Roux wrote:
> Hi Gil,
>
> Here are my preferences inline...
>
> Le 20/07/2022 à 23:49, Gil Portenseigne a écrit :
> > Thanks All for the feedback.
> >
> > I continue in my spare time and did analyse some rule that I propose to disable.
> > Here are my thought :
> >
> > * DuplicateStringLiteral : Not Adapted to OFBiz : String Literal are massively entityName. No need to make them constants.
>
> +1
>
> > * LineLength : Can be configured : default 120, with 140 configured there are 654 fix needed, 445 fixes to 150 length configuration (same as Java checkstyle). IMO I prefer default config, but we could go with the same as checkstyle config ?
> 150
> > * UnnecessaryGetter : Lot of java OFBiz code use getXXX. Applying this rule, will lead massive java changes. I prefer to ignore.
> +1
> > * NoDef : We need to agree to not use def in variable declaration or method return type (implies lots of fix). I think that is better to have type defined.
> +1
> > * MethodReturnTypeRequired : same as above
> +1
> > * UnnecessaryReturnKeyword : Not sure that is is wanted, in our dev team it can be disturbing, *WDYT* ?
> Better to have return, clearer
> > * UnnecessaryObjectReferences : with method usage to reduce code. Context.with { toto = « toto »}, I was unaware of this, i'd like to keep it if possible.
> +1
> > * CompileStatic : I propose to ignore this rule, not needed IMO
> +1
> > IfStatementBraces : Do we allow one lined if without braces ? I prefer not, but that seems convenient some times, This rule applies also on multiline, and for me we should keep it. Since it is not configurable for oneline, i am into keeping it.
> +1
> > * DuplicateNumberLiteral : Same as String Literal
> +1
> > * UnnecessarySetter : Same as UnnecessaryGetter
>
> +1
>
> Thanks
>
> Jacques
>
> >
> > Thanks,
> >
> > Gil
> >
> > On 2022/07/04 15:24:43 Gil Portenseigne wrote:
> > > Hello Devs,
> > >
> > > I would like to continue Codenarc integration in OFBiz (OFBIZ-11167).
> > >
> > > For those who are not aware, Codenarc is an analysis tools for Groovy
> > > for defects, bad practices, inconsistencies, style issues and more.
> > >
> > > For this purpose we need to agree about the ruleset to put in place.
> > >
> > > I took as a basis the ruleset :
> > > https://codenarc.org/StarterRuleSet-AllRulesByCategory.groovy.txt
> > >
> > > And started to fix some in https://github.com/apache/ofbiz-framework/pull/517
> > >
> > > Before doing the complete work, a first rule made me wonder if that is a
> > > choice that we will be doing as a community :
> > >
> > > IfStatementBraces - Use braces for if statements, even for a single statement.
> > >
> > > There are 234 occurrences in the project, and I guess that some opinions
> > > might diverge on this subject.
> > >
> > > Moreover, some rules needs lots of fixes in the project (those with more
> > > than 200 occurrences) :
> > >
> > > +------------+---------------------------------------------------------------------+
> > > | Number of | Rule name and details |
> > > | occurrence | |
> > > +============+=====================================================================+
> > > | 9883 | UnnecessaryGString - String objects should be created with single |
> > > | | quotes, and GString objects created with double quotes. Creating |
> > > | | normal String objects with double quotes is confusing to readers. |
> > > +------------+---------------------------------------------------------------------+
> > > | 4569 | DuplicateStringLiteral - Code containing duplicate String literals |
> > > | | can usually be improved by declaring the String as a constant |
> > > | | field. The ignoreStrings property () can optionally specify a |
> > > | | comma-separated list of Strings to ignore. |
> > > +------------+---------------------------------------------------------------------+
> > > | 4209 | SpaceAroundMapEntryColon - Check for configured formatting of |
> > > | | whitespace around colons for literal Map entries. |
> > > | | The characterBeforeColonRegex and characterAfterColonRegex |
> > > | | properties specify a regular expression that must match the |
> > > | | character before/after the colon. |
> > > +------------+---------------------------------------------------------------------+
> > > | 1448 | LineLength - Checks the maximum length for each line of |
> > > | | source code. It checks for number of characters, so lines |
> > > | | that include tabs may appear longer than the allowed number |
> > > | | when viewing the file. The maximum line length can be |
> > > | | configured by setting the length property, which defaults to 120. |
> > > +------------+---------------------------------------------------------------------+
> > > | 885 | UnnecessaryGetter - Checks for explicit calls to getter/accessor |
> > > | | methods which can, for the most part, be replaced by property |
> > > | | access. A getter is defined as a method call that matches |
> > > | | get[A-Z] but not getClass() or get[A-Z][A-Z] such as getURL(). |
> > > | | Getters do not take method arguments. The ignoreMethodNames |
> > > | | property (null) can specify method names that should be ignored |
> > > | | , optionally containing wildcard characters ('*' or '?'). |
> > > +------------+---------------------------------------------------------------------+
> > > | 601 | NoDef - def should not be used. You should replace it with |
> > > | | concrete type. |
> > > +------------+---------------------------------------------------------------------+
> > > | 485 | MethodReturnTypeRequired - Checks that method return types |
> > > | | are not dynamic, that is they are explicitly stated and |
> > > | | different than def. |
> > > +------------+---------------------------------------------------------------------+
> > > | 484 | Indentation - Check indentation for class and method |
> > > | | declarations, and initial statements. |
> > > +------------+---------------------------------------------------------------------+
> > > | 482 | UnnecessaryReturnKeyword - In Groovy, the return keyword |
> > > | | is often optional. If a statement is the last line in a |
> > > | | method or closure then you do not need to have the return keyword. |
> > > +------------+---------------------------------------------------------------------+
> > > | 407 | UnnecessaryObjectReferences - Violations are triggered when |
> > > | | an excessive set of consecutive statements all reference |
> > > | | the same variable. This can be made more readable by using |
> > > | | a with or identity block. |
> > > +------------+---------------------------------------------------------------------+
> > > | 345 | CompileStatic - Check that classes are explicitely annotated |
> > > | | with either @GrailsCompileStatic, @CompileStatic or @CompileDynamic |
> > > +------------+---------------------------------------------------------------------+
> > > | 320 | ExplicitCallToEqualsMethod - This rule detects when the |
> > > | | equals(Object) method is called directly in code instead |
> > > | | of using the == or != operator. A groovier way to |
> > > | | express this: a.equals(b) is this: a == b and a groovier |
> > > | | way to express : !a.equals(b) is : a != b |
> > > +------------+---------------------------------------------------------------------+
> > > | 235 | IfStatementBraces - Use braces for if statements, |
> > > | | even for a single statement. |
> > > +------------+---------------------------------------------------------------------+
> > > | 235 | SpaceAroundOperator - Check that there is at least one space |
> > > | | (blank) or whitespace around each binary operator. |
> > > +------------+---------------------------------------------------------------------+
> > > | 211 | NestedBlockDepth - Checks for blocks or closures nested more |
> > > | | than maxNestedBlockDepth (5) levels deep. |
> > > +------------+---------------------------------------------------------------------+
> > > | 201 | TrailingWhitespace - Checks that no lines of source code |
> > > | | end with whitespace characters. |
> > > +------------+---------------------------------------------------------------------+
> > >
> > > I think that if someone want to express an opposition about applying one
> > > of the above rule, i would be great to express it and discuss it here.
> > >
> > > My opinion is that it could be nice to adopt every rules, to respect
> > > standard best practice. Even if it implies lot of code changes.
> > >
> > > Thanks in advance for your feedback.
> > >
> > > Regards,
> > >
> > > Gil
> > >
Re: Codenarc integration, rules to use.
Posted by Jacques Le Roux <ja...@les7arts.com>.
Hi Gil,
Here are my preferences inline...
Le 20/07/2022 à 23:49, Gil Portenseigne a écrit :
> Thanks All for the feedback.
>
> I continue in my spare time and did analyse some rule that I propose to disable.
> Here are my thought :
>
> * DuplicateStringLiteral : Not Adapted to OFBiz : String Literal are massively entityName. No need to make them constants.
+1
> * LineLength : Can be configured : default 120, with 140 configured there are 654 fix needed, 445 fixes to 150 length configuration (same as Java checkstyle). IMO I prefer default config, but we could go with the same as checkstyle config ?
150
> * UnnecessaryGetter : Lot of java OFBiz code use getXXX. Applying this rule, will lead massive java changes. I prefer to ignore.
+1
> * NoDef : We need to agree to not use def in variable declaration or method return type (implies lots of fix). I think that is better to have type defined.
+1
> * MethodReturnTypeRequired : same as above
+1
> * UnnecessaryReturnKeyword : Not sure that is is wanted, in our dev team it can be disturbing, *WDYT* ?
Better to have return, clearer
> * UnnecessaryObjectReferences : with method usage to reduce code. Context.with { toto = « toto »}, I was unaware of this, i'd like to keep it if possible.
+1
> * CompileStatic : I propose to ignore this rule, not needed IMO
+1
> IfStatementBraces : Do we allow one lined if without braces ? I prefer not, but that seems convenient some times, This rule applies also on multiline, and for me we should keep it. Since it is not configurable for oneline, i am into keeping it.
+1
> * DuplicateNumberLiteral : Same as String Literal
+1
> * UnnecessarySetter : Same as UnnecessaryGetter
+1
Thanks
Jacques
>
> Thanks,
>
> Gil
>
> On 2022/07/04 15:24:43 Gil Portenseigne wrote:
>> Hello Devs,
>>
>> I would like to continue Codenarc integration in OFBiz (OFBIZ-11167).
>>
>> For those who are not aware, Codenarc is an analysis tools for Groovy
>> for defects, bad practices, inconsistencies, style issues and more.
>>
>> For this purpose we need to agree about the ruleset to put in place.
>>
>> I took as a basis the ruleset :
>> https://codenarc.org/StarterRuleSet-AllRulesByCategory.groovy.txt
>>
>> And started to fix some in https://github.com/apache/ofbiz-framework/pull/517
>>
>> Before doing the complete work, a first rule made me wonder if that is a
>> choice that we will be doing as a community :
>>
>> IfStatementBraces - Use braces for if statements, even for a single statement.
>>
>> There are 234 occurrences in the project, and I guess that some opinions
>> might diverge on this subject.
>>
>> Moreover, some rules needs lots of fixes in the project (those with more
>> than 200 occurrences) :
>>
>> +------------+---------------------------------------------------------------------+
>> | Number of | Rule name and details |
>> | occurrence | |
>> +============+=====================================================================+
>> | 9883 | UnnecessaryGString - String objects should be created with single |
>> | | quotes, and GString objects created with double quotes. Creating |
>> | | normal String objects with double quotes is confusing to readers. |
>> +------------+---------------------------------------------------------------------+
>> | 4569 | DuplicateStringLiteral - Code containing duplicate String literals |
>> | | can usually be improved by declaring the String as a constant |
>> | | field. The ignoreStrings property () can optionally specify a |
>> | | comma-separated list of Strings to ignore. |
>> +------------+---------------------------------------------------------------------+
>> | 4209 | SpaceAroundMapEntryColon - Check for configured formatting of |
>> | | whitespace around colons for literal Map entries. |
>> | | The characterBeforeColonRegex and characterAfterColonRegex |
>> | | properties specify a regular expression that must match the |
>> | | character before/after the colon. |
>> +------------+---------------------------------------------------------------------+
>> | 1448 | LineLength - Checks the maximum length for each line of |
>> | | source code. It checks for number of characters, so lines |
>> | | that include tabs may appear longer than the allowed number |
>> | | when viewing the file. The maximum line length can be |
>> | | configured by setting the length property, which defaults to 120. |
>> +------------+---------------------------------------------------------------------+
>> | 885 | UnnecessaryGetter - Checks for explicit calls to getter/accessor |
>> | | methods which can, for the most part, be replaced by property |
>> | | access. A getter is defined as a method call that matches |
>> | | get[A-Z] but not getClass() or get[A-Z][A-Z] such as getURL(). |
>> | | Getters do not take method arguments. The ignoreMethodNames |
>> | | property (null) can specify method names that should be ignored |
>> | | , optionally containing wildcard characters ('*' or '?'). |
>> +------------+---------------------------------------------------------------------+
>> | 601 | NoDef - def should not be used. You should replace it with |
>> | | concrete type. |
>> +------------+---------------------------------------------------------------------+
>> | 485 | MethodReturnTypeRequired - Checks that method return types |
>> | | are not dynamic, that is they are explicitly stated and |
>> | | different than def. |
>> +------------+---------------------------------------------------------------------+
>> | 484 | Indentation - Check indentation for class and method |
>> | | declarations, and initial statements. |
>> +------------+---------------------------------------------------------------------+
>> | 482 | UnnecessaryReturnKeyword - In Groovy, the return keyword |
>> | | is often optional. If a statement is the last line in a |
>> | | method or closure then you do not need to have the return keyword. |
>> +------------+---------------------------------------------------------------------+
>> | 407 | UnnecessaryObjectReferences - Violations are triggered when |
>> | | an excessive set of consecutive statements all reference |
>> | | the same variable. This can be made more readable by using |
>> | | a with or identity block. |
>> +------------+---------------------------------------------------------------------+
>> | 345 | CompileStatic - Check that classes are explicitely annotated |
>> | | with either @GrailsCompileStatic, @CompileStatic or @CompileDynamic |
>> +------------+---------------------------------------------------------------------+
>> | 320 | ExplicitCallToEqualsMethod - This rule detects when the |
>> | | equals(Object) method is called directly in code instead |
>> | | of using the == or != operator. A groovier way to |
>> | | express this: a.equals(b) is this: a == b and a groovier |
>> | | way to express : !a.equals(b) is : a != b |
>> +------------+---------------------------------------------------------------------+
>> | 235 | IfStatementBraces - Use braces for if statements, |
>> | | even for a single statement. |
>> +------------+---------------------------------------------------------------------+
>> | 235 | SpaceAroundOperator - Check that there is at least one space |
>> | | (blank) or whitespace around each binary operator. |
>> +------------+---------------------------------------------------------------------+
>> | 211 | NestedBlockDepth - Checks for blocks or closures nested more |
>> | | than maxNestedBlockDepth (5) levels deep. |
>> +------------+---------------------------------------------------------------------+
>> | 201 | TrailingWhitespace - Checks that no lines of source code |
>> | | end with whitespace characters. |
>> +------------+---------------------------------------------------------------------+
>>
>> I think that if someone want to express an opposition about applying one
>> of the above rule, i would be great to express it and discuss it here.
>>
>> My opinion is that it could be nice to adopt every rules, to respect
>> standard best practice. Even if it implies lot of code changes.
>>
>> Thanks in advance for your feedback.
>>
>> Regards,
>>
>> Gil
>>
Re: Codenarc integration, rules to use.
Posted by Gil Portenseigne <pg...@apache.org>.
Thanks All for the feedback.
I continue in my spare time and did analyse some rule that I propose to disable.
Here are my thought :
* DuplicateStringLiteral : Not Adapted to OFBiz : String Literal are massively entityName. No need to make them constants.
* LineLength : Can be configured : default 120, with 140 configured there are 654 fix needed, 445 fixes to 150 length configuration (same as Java checkstyle). IMO I prefer default config, but we could go with the same as checkstyle config ?
* UnnecessaryGetter : Lot of java OFBiz code use getXXX. Applying this rule, will lead massive java changes. I prefer to ignore.
* NoDef : We need to agree to not use def in variable declaration or method return type (implies lots of fix). I think that is better to have type defined.
* MethodReturnTypeRequired : same as above
* UnnecessaryReturnKeyword : Not sure that is is wanted, in our dev team it can be disturbing, *WDYT* ?
* UnnecessaryObjectReferences : with method usage to reduce code. Context.with { toto = « toto »}, I was unaware of this, i'd like to keep it if possible.
* CompileStatic : I propose to ignore this rule, not needed IMO
IfStatementBraces : Do we allow one lined if without braces ? I prefer not, but that seems convenient some times, This rule applies also on multiline, and for me we should keep it. Since it is not configurable for oneline, i am into keeping it.
* DuplicateNumberLiteral : Same as String Literal
* UnnecessarySetter : Same as UnnecessaryGetter
Thanks,
Gil
On 2022/07/04 15:24:43 Gil Portenseigne wrote:
> Hello Devs,
>
> I would like to continue Codenarc integration in OFBiz (OFBIZ-11167).
>
> For those who are not aware, Codenarc is an analysis tools for Groovy
> for defects, bad practices, inconsistencies, style issues and more.
>
> For this purpose we need to agree about the ruleset to put in place.
>
> I took as a basis the ruleset :
> https://codenarc.org/StarterRuleSet-AllRulesByCategory.groovy.txt
>
> And started to fix some in https://github.com/apache/ofbiz-framework/pull/517
>
> Before doing the complete work, a first rule made me wonder if that is a
> choice that we will be doing as a community :
>
> IfStatementBraces - Use braces for if statements, even for a single statement.
>
> There are 234 occurrences in the project, and I guess that some opinions
> might diverge on this subject.
>
> Moreover, some rules needs lots of fixes in the project (those with more
> than 200 occurrences) :
>
> +------------+---------------------------------------------------------------------+
> | Number of | Rule name and details |
> | occurrence | |
> +============+=====================================================================+
> | 9883 | UnnecessaryGString - String objects should be created with single |
> | | quotes, and GString objects created with double quotes. Creating |
> | | normal String objects with double quotes is confusing to readers. |
> +------------+---------------------------------------------------------------------+
> | 4569 | DuplicateStringLiteral - Code containing duplicate String literals |
> | | can usually be improved by declaring the String as a constant |
> | | field. The ignoreStrings property () can optionally specify a |
> | | comma-separated list of Strings to ignore. |
> +------------+---------------------------------------------------------------------+
> | 4209 | SpaceAroundMapEntryColon - Check for configured formatting of |
> | | whitespace around colons for literal Map entries. |
> | | The characterBeforeColonRegex and characterAfterColonRegex |
> | | properties specify a regular expression that must match the |
> | | character before/after the colon. |
> +------------+---------------------------------------------------------------------+
> | 1448 | LineLength - Checks the maximum length for each line of |
> | | source code. It checks for number of characters, so lines |
> | | that include tabs may appear longer than the allowed number |
> | | when viewing the file. The maximum line length can be |
> | | configured by setting the length property, which defaults to 120. |
> +------------+---------------------------------------------------------------------+
> | 885 | UnnecessaryGetter - Checks for explicit calls to getter/accessor |
> | | methods which can, for the most part, be replaced by property |
> | | access. A getter is defined as a method call that matches |
> | | get[A-Z] but not getClass() or get[A-Z][A-Z] such as getURL(). |
> | | Getters do not take method arguments. The ignoreMethodNames |
> | | property (null) can specify method names that should be ignored |
> | | , optionally containing wildcard characters ('*' or '?'). |
> +------------+---------------------------------------------------------------------+
> | 601 | NoDef - def should not be used. You should replace it with |
> | | concrete type. |
> +------------+---------------------------------------------------------------------+
> | 485 | MethodReturnTypeRequired - Checks that method return types |
> | | are not dynamic, that is they are explicitly stated and |
> | | different than def. |
> +------------+---------------------------------------------------------------------+
> | 484 | Indentation - Check indentation for class and method |
> | | declarations, and initial statements. |
> +------------+---------------------------------------------------------------------+
> | 482 | UnnecessaryReturnKeyword - In Groovy, the return keyword |
> | | is often optional. If a statement is the last line in a |
> | | method or closure then you do not need to have the return keyword. |
> +------------+---------------------------------------------------------------------+
> | 407 | UnnecessaryObjectReferences - Violations are triggered when |
> | | an excessive set of consecutive statements all reference |
> | | the same variable. This can be made more readable by using |
> | | a with or identity block. |
> +------------+---------------------------------------------------------------------+
> | 345 | CompileStatic - Check that classes are explicitely annotated |
> | | with either @GrailsCompileStatic, @CompileStatic or @CompileDynamic |
> +------------+---------------------------------------------------------------------+
> | 320 | ExplicitCallToEqualsMethod - This rule detects when the |
> | | equals(Object) method is called directly in code instead |
> | | of using the == or != operator. A groovier way to |
> | | express this: a.equals(b) is this: a == b and a groovier |
> | | way to express : !a.equals(b) is : a != b |
> +------------+---------------------------------------------------------------------+
> | 235 | IfStatementBraces - Use braces for if statements, |
> | | even for a single statement. |
> +------------+---------------------------------------------------------------------+
> | 235 | SpaceAroundOperator - Check that there is at least one space |
> | | (blank) or whitespace around each binary operator. |
> +------------+---------------------------------------------------------------------+
> | 211 | NestedBlockDepth - Checks for blocks or closures nested more |
> | | than maxNestedBlockDepth (5) levels deep. |
> +------------+---------------------------------------------------------------------+
> | 201 | TrailingWhitespace - Checks that no lines of source code |
> | | end with whitespace characters. |
> +------------+---------------------------------------------------------------------+
>
> I think that if someone want to express an opposition about applying one
> of the above rule, i would be great to express it and discuss it here.
>
> My opinion is that it could be nice to adopt every rules, to respect
> standard best practice. Even if it implies lot of code changes.
>
> Thanks in advance for your feedback.
>
> Regards,
>
> Gil
>
Re: Codenarc integration, rules to use.
Posted by Jacopo Cappellato <ja...@gmail.com>.
+1
Thank you Gil!
Jacopo
On Mon, Jul 4, 2022 at 5:24 PM Gil Portenseigne <gi...@nereide.fr>
wrote:
> Hello Devs,
>
> I would like to continue Codenarc integration in OFBiz (OFBIZ-11167).
>
> For those who are not aware, Codenarc is an analysis tools for Groovy
> for defects, bad practices, inconsistencies, style issues and more.
>
> For this purpose we need to agree about the ruleset to put in place.
>
> I took as a basis the ruleset :
> https://codenarc.org/StarterRuleSet-AllRulesByCategory.groovy.txt
>
> And started to fix some in
> https://github.com/apache/ofbiz-framework/pull/517
>
> Before doing the complete work, a first rule made me wonder if that is a
> choice that we will be doing as a community :
>
> IfStatementBraces - Use braces for if statements, even for a single
> statement.
>
> There are 234 occurrences in the project, and I guess that some opinions
> might diverge on this subject.
>
> Moreover, some rules needs lots of fixes in the project (those with more
> than 200 occurrences) :
>
>
> +------------+---------------------------------------------------------------------+
> | Number of | Rule name and details
> |
> | occurrence |
> |
>
> +============+=====================================================================+
> | 9883 | UnnecessaryGString - String objects should be created with
> single |
> | | quotes, and GString objects created with double quotes.
> Creating |
> | | normal String objects with double quotes is confusing to
> readers. |
>
> +------------+---------------------------------------------------------------------+
> | 4569 | DuplicateStringLiteral - Code containing duplicate String
> literals |
> | | can usually be improved by declaring the String as a
> constant |
> | | field. The ignoreStrings property () can optionally
> specify a |
> | | comma-separated list of Strings to ignore.
> |
>
> +------------+---------------------------------------------------------------------+
> | 4209 | SpaceAroundMapEntryColon - Check for configured formatting
> of |
> | | whitespace around colons for literal Map entries.
> |
> | | The characterBeforeColonRegex and
> characterAfterColonRegex |
> | | properties specify a regular expression that must match
> the |
> | | character before/after the colon.
> |
>
> +------------+---------------------------------------------------------------------+
> | 1448 | LineLength - Checks the maximum length for each line of
> |
> | | source code. It checks for number of characters, so lines
> |
> | | that include tabs may appear longer than the allowed
> number |
> | | when viewing the file. The maximum line length can be
> |
> | | configured by setting the length property, which defaults
> to 120. |
>
> +------------+---------------------------------------------------------------------+
> | 885 | UnnecessaryGetter - Checks for explicit calls to
> getter/accessor |
> | | methods which can, for the most part, be replaced by
> property |
> | | access. A getter is defined as a method call that
> matches |
> | | get[A-Z] but not getClass() or get[A-Z][A-Z] such as
> getURL(). |
> | | Getters do not take method arguments. The
> ignoreMethodNames |
> | | property (null) can specify method names that should be
> ignored |
> | | , optionally containing wildcard characters ('*' or '?').
> |
>
> +------------+---------------------------------------------------------------------+
> | 601 | NoDef - def should not be used. You should replace it with
> |
> | | concrete type.
> |
>
> +------------+---------------------------------------------------------------------+
> | 485 | MethodReturnTypeRequired - Checks that method return
> types |
> | | are not dynamic, that is they are explicitly stated and
> |
> | | different than def.
> |
>
> +------------+---------------------------------------------------------------------+
> | 484 | Indentation - Check indentation for class and method
> |
> | | declarations, and initial statements.
> |
>
> +------------+---------------------------------------------------------------------+
> | 482 | UnnecessaryReturnKeyword - In Groovy, the return keyword
> |
> | | is often optional. If a statement is the last line in a
> |
> | | method or closure then you do not need to have the return
> keyword. |
>
> +------------+---------------------------------------------------------------------+
> | 407 | UnnecessaryObjectReferences - Violations are triggered
> when |
> | | an excessive set of consecutive statements all reference
> |
> | | the same variable. This can be made more readable by
> using |
> | | a with or identity block.
> |
>
> +------------+---------------------------------------------------------------------+
> | 345 | CompileStatic - Check that classes are explicitely
> annotated |
> | | with either @GrailsCompileStatic, @CompileStatic or
> @CompileDynamic |
>
> +------------+---------------------------------------------------------------------+
> | 320 | ExplicitCallToEqualsMethod - This rule detects when the
> |
> | | equals(Object) method is called directly in code instead
> |
> | | of using the == or != operator. A groovier way to
> |
> | | express this: a.equals(b) is this: a == b and a groovier
> |
> | | way to express : !a.equals(b) is : a != b
> |
>
> +------------+---------------------------------------------------------------------+
> | 235 | IfStatementBraces - Use braces for if statements,
> |
> | | even for a single statement.
> |
>
> +------------+---------------------------------------------------------------------+
> | 235 | SpaceAroundOperator - Check that there is at least one
> space |
> | | (blank) or whitespace around each binary operator.
> |
>
> +------------+---------------------------------------------------------------------+
> | 211 | NestedBlockDepth - Checks for blocks or closures nested
> more |
> | | than maxNestedBlockDepth (5) levels deep.
> |
>
> +------------+---------------------------------------------------------------------+
> | 201 | TrailingWhitespace - Checks that no lines of source code
> |
> | | end with whitespace characters.
> |
>
> +------------+---------------------------------------------------------------------+
>
> I think that if someone want to express an opposition about applying one
> of the above rule, i would be great to express it and discuss it here.
>
> My opinion is that it could be nice to adopt every rules, to respect
> standard best practice. Even if it implies lot of code changes.
>
> Thanks in advance for your feedback.
>
> Regards,
>
> Gil
>
Re: Codenarc integration, rules to use.
Posted by "gil.portenseigne@nereide.fr" <gi...@nereide.fr>.
Hello Michael,
Sure, you can see in the pull request I started to separate the commit, a commit for one rule fix.
The last one is a big one :)
https://github.com/apache/ofbiz-framework/pull/517/commits/384894dfafce5e7261e2564b40261650deda7a22
Regards,
Gil
Le Dimanche, Juillet 10, 2022 16:02 CEST, Michael Brohl <mi...@ecomify.de> a écrit:
Hi Gil,
I was on vacation so a bit late: I fully agree to apply these rules.
We should, however, encourage contributors to apply those changes in
separate commits which ONLY contain those changes and not mix it up with
other changes to make review work easier.
Thanks for the initiative,
Michael Brohl
ecomify GmbH - www.ecomify.de
Am 04.07.22 um 17:24 schrieb Gil Portenseigne:
> Hello Devs,
>
> I would like to continue Codenarc integration in OFBiz (OFBIZ-11167).
>
> For those who are not aware, Codenarc is an analysis tools for Groovy
> for defects, bad practices, inconsistencies, style issues and more.
>
> For this purpose we need to agree about the ruleset to put in place.
>
> I took as a basis the ruleset :
> https://codenarc.org/StarterRuleSet-AllRulesByCategory.groovy.txt
>
> And started to fix some in https://github.com/apache/ofbiz-framework/pull/517
>
> Before doing the complete work, a first rule made me wonder if that is a
> choice that we will be doing as a community :
>
> IfStatementBraces - Use braces for if statements, even for a single statement.
>
> There are 234 occurrences in the project, and I guess that some opinions
> might diverge on this subject.
>
> Moreover, some rules needs lots of fixes in the project (those with more
> than 200 occurrences) :
>
> +------------+---------------------------------------------------------------------+
> | Number of | Rule name and details |
> | occurrence | |
> +============+=====================================================================+
> | 9883 | UnnecessaryGString - String objects should be created with single |
> | | quotes, and GString objects created with double quotes. Creating |
> | | normal String objects with double quotes is confusing to readers. |
> +------------+---------------------------------------------------------------------+
> | 4569 | DuplicateStringLiteral - Code containing duplicate String literals |
> | | can usually be improved by declaring the String as a constant |
> | | field. The ignoreStrings property () can optionally specify a |
> | | comma-separated list of Strings to ignore. |
> +------------+---------------------------------------------------------------------+
> | 4209 | SpaceAroundMapEntryColon - Check for configured formatting of |
> | | whitespace around colons for literal Map entries. |
> | | The characterBeforeColonRegex and characterAfterColonRegex |
> | | properties specify a regular expression that must match the |
> | | character before/after the colon. |
> +------------+---------------------------------------------------------------------+
> | 1448 | LineLength - Checks the maximum length for each line of |
> | | source code. It checks for number of characters, so lines |
> | | that include tabs may appear longer than the allowed number |
> | | when viewing the file. The maximum line length can be |
> | | configured by setting the length property, which defaults to 120. |
> +------------+---------------------------------------------------------------------+
> | 885 | UnnecessaryGetter - Checks for explicit calls to getter/accessor |
> | | methods which can, for the most part, be replaced by property |
> | | access. A getter is defined as a method call that matches |
> | | get[A-Z] but not getClass() or get[A-Z][A-Z] such as getURL(). |
> | | Getters do not take method arguments. The ignoreMethodNames |
> | | property (null) can specify method names that should be ignored |
> | | , optionally containing wildcard characters ('*' or '?'). |
> +------------+---------------------------------------------------------------------+
> | 601 | NoDef - def should not be used. You should replace it with |
> | | concrete type. |
> +------------+---------------------------------------------------------------------+
> | 485 | MethodReturnTypeRequired - Checks that method return types |
> | | are not dynamic, that is they are explicitly stated and |
> | | different than def. |
> +------------+---------------------------------------------------------------------+
> | 484 | Indentation - Check indentation for class and method |
> | | declarations, and initial statements. |
> +------------+---------------------------------------------------------------------+
> | 482 | UnnecessaryReturnKeyword - In Groovy, the return keyword |
> | | is often optional. If a statement is the last line in a |
> | | method or closure then you do not need to have the return keyword. |
> +------------+---------------------------------------------------------------------+
> | 407 | UnnecessaryObjectReferences - Violations are triggered when |
> | | an excessive set of consecutive statements all reference |
> | | the same variable. This can be made more readable by using |
> | | a with or identity block. |
> +------------+---------------------------------------------------------------------+
> | 345 | CompileStatic - Check that classes are explicitely annotated |
> | | with either @GrailsCompileStatic, @CompileStatic or @CompileDynamic |
> +------------+---------------------------------------------------------------------+
> | 320 | ExplicitCallToEqualsMethod - This rule detects when the |
> | | equals(Object) method is called directly in code instead |
> | | of using the == or != operator. A groovier way to |
> | | express this: a.equals(b) is this: a == b and a groovier |
> | | way to express : !a.equals(b) is : a != b |
> +------------+---------------------------------------------------------------------+
> | 235 | IfStatementBraces - Use braces for if statements, |
> | | even for a single statement. |
> +------------+---------------------------------------------------------------------+
> | 235 | SpaceAroundOperator - Check that there is at least one space |
> | | (blank) or whitespace around each binary operator. |
> +------------+---------------------------------------------------------------------+
> | 211 | NestedBlockDepth - Checks for blocks or closures nested more |
> | | than maxNestedBlockDepth (5) levels deep. |
> +------------+---------------------------------------------------------------------+
> | 201 | TrailingWhitespace - Checks that no lines of source code |
> | | end with whitespace characters. |
> +------------+---------------------------------------------------------------------+
>
> I think that if someone want to express an opposition about applying one
> of the above rule, i would be great to express it and discuss it here.
>
> My opinion is that it could be nice to adopt every rules, to respect
> standard best practice. Even if it implies lot of code changes.
>
> Thanks in advance for your feedback.
>
> Regards,
>
> Gil
Re: Codenarc integration, rules to use.
Posted by Michael Brohl <mi...@ecomify.de>.
Hi Gil,
I was on vacation so a bit late: I fully agree to apply these rules.
We should, however, encourage contributors to apply those changes in
separate commits which ONLY contain those changes and not mix it up with
other changes to make review work easier.
Thanks for the initiative,
Michael Brohl
ecomify GmbH - www.ecomify.de
Am 04.07.22 um 17:24 schrieb Gil Portenseigne:
> Hello Devs,
>
> I would like to continue Codenarc integration in OFBiz (OFBIZ-11167).
>
> For those who are not aware, Codenarc is an analysis tools for Groovy
> for defects, bad practices, inconsistencies, style issues and more.
>
> For this purpose we need to agree about the ruleset to put in place.
>
> I took as a basis the ruleset :
> https://codenarc.org/StarterRuleSet-AllRulesByCategory.groovy.txt
>
> And started to fix some in https://github.com/apache/ofbiz-framework/pull/517
>
> Before doing the complete work, a first rule made me wonder if that is a
> choice that we will be doing as a community :
>
> IfStatementBraces - Use braces for if statements, even for a single statement.
>
> There are 234 occurrences in the project, and I guess that some opinions
> might diverge on this subject.
>
> Moreover, some rules needs lots of fixes in the project (those with more
> than 200 occurrences) :
>
> +------------+---------------------------------------------------------------------+
> | Number of | Rule name and details |
> | occurrence | |
> +============+=====================================================================+
> | 9883 | UnnecessaryGString - String objects should be created with single |
> | | quotes, and GString objects created with double quotes. Creating |
> | | normal String objects with double quotes is confusing to readers. |
> +------------+---------------------------------------------------------------------+
> | 4569 | DuplicateStringLiteral - Code containing duplicate String literals |
> | | can usually be improved by declaring the String as a constant |
> | | field. The ignoreStrings property () can optionally specify a |
> | | comma-separated list of Strings to ignore. |
> +------------+---------------------------------------------------------------------+
> | 4209 | SpaceAroundMapEntryColon - Check for configured formatting of |
> | | whitespace around colons for literal Map entries. |
> | | The characterBeforeColonRegex and characterAfterColonRegex |
> | | properties specify a regular expression that must match the |
> | | character before/after the colon. |
> +------------+---------------------------------------------------------------------+
> | 1448 | LineLength - Checks the maximum length for each line of |
> | | source code. It checks for number of characters, so lines |
> | | that include tabs may appear longer than the allowed number |
> | | when viewing the file. The maximum line length can be |
> | | configured by setting the length property, which defaults to 120. |
> +------------+---------------------------------------------------------------------+
> | 885 | UnnecessaryGetter - Checks for explicit calls to getter/accessor |
> | | methods which can, for the most part, be replaced by property |
> | | access. A getter is defined as a method call that matches |
> | | get[A-Z] but not getClass() or get[A-Z][A-Z] such as getURL(). |
> | | Getters do not take method arguments. The ignoreMethodNames |
> | | property (null) can specify method names that should be ignored |
> | | , optionally containing wildcard characters ('*' or '?'). |
> +------------+---------------------------------------------------------------------+
> | 601 | NoDef - def should not be used. You should replace it with |
> | | concrete type. |
> +------------+---------------------------------------------------------------------+
> | 485 | MethodReturnTypeRequired - Checks that method return types |
> | | are not dynamic, that is they are explicitly stated and |
> | | different than def. |
> +------------+---------------------------------------------------------------------+
> | 484 | Indentation - Check indentation for class and method |
> | | declarations, and initial statements. |
> +------------+---------------------------------------------------------------------+
> | 482 | UnnecessaryReturnKeyword - In Groovy, the return keyword |
> | | is often optional. If a statement is the last line in a |
> | | method or closure then you do not need to have the return keyword. |
> +------------+---------------------------------------------------------------------+
> | 407 | UnnecessaryObjectReferences - Violations are triggered when |
> | | an excessive set of consecutive statements all reference |
> | | the same variable. This can be made more readable by using |
> | | a with or identity block. |
> +------------+---------------------------------------------------------------------+
> | 345 | CompileStatic - Check that classes are explicitely annotated |
> | | with either @GrailsCompileStatic, @CompileStatic or @CompileDynamic |
> +------------+---------------------------------------------------------------------+
> | 320 | ExplicitCallToEqualsMethod - This rule detects when the |
> | | equals(Object) method is called directly in code instead |
> | | of using the == or != operator. A groovier way to |
> | | express this: a.equals(b) is this: a == b and a groovier |
> | | way to express : !a.equals(b) is : a != b |
> +------------+---------------------------------------------------------------------+
> | 235 | IfStatementBraces - Use braces for if statements, |
> | | even for a single statement. |
> +------------+---------------------------------------------------------------------+
> | 235 | SpaceAroundOperator - Check that there is at least one space |
> | | (blank) or whitespace around each binary operator. |
> +------------+---------------------------------------------------------------------+
> | 211 | NestedBlockDepth - Checks for blocks or closures nested more |
> | | than maxNestedBlockDepth (5) levels deep. |
> +------------+---------------------------------------------------------------------+
> | 201 | TrailingWhitespace - Checks that no lines of source code |
> | | end with whitespace characters. |
> +------------+---------------------------------------------------------------------+
>
> I think that if someone want to express an opposition about applying one
> of the above rule, i would be great to express it and discuss it here.
>
> My opinion is that it could be nice to adopt every rules, to respect
> standard best practice. Even if it implies lot of code changes.
>
> Thanks in advance for your feedback.
>
> Regards,
>
> Gil