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
> > >
signature.asc
Description: PGP signature
