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
