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
>
