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 <lekt...@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 <gil.portensei...@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é.
