Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Codenarc integration #517

Closed
wants to merge 145 commits into from
Closed

Codenarc integration #517

wants to merge 145 commits into from

Conversation

gilPts
Copy link
Contributor

@gilPts gilPts commented Jul 1, 2022

Implemented: add codenarc and fix first basic rules.
(OFBIZ-11167)

@gilPts gilPts force-pushed the codenarc_integration branch 2 times, most recently from 70a2257 to 04aa74d Compare July 16, 2022 23:00
@sonarcloud
Copy link

sonarcloud bot commented Sep 9, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
9.3% 9.3% Duplication

@gilPts gilPts force-pushed the codenarc_integration branch 8 times, most recently from 19d8633 to 7ee3e84 Compare January 13, 2023 16:57
@gilPts gilPts marked this pull request as ready for review January 20, 2023 14:50
@gilPts gilPts force-pushed the codenarc_integration branch 2 times, most recently from 7528866 to 8146b94 Compare January 20, 2023 16:23
@gilPts
Implemented : add codenarc plugin usage with configuration based on s…
bacafa8 
…tarter Ruleset (OFBIZ-11167)

Customize SpaceAroundMapEntryColon rule to respect community opinion
Map in OFBiz groovy should respect [key: value] syntax

Add LineLength parameter to 150 length

Remove TrailingComma codenarc rule : Check whether list and map literals
contain optional trailing comma.

Remove DuplicateString/NumberLiteral codenarc rules : since we are not using
OOP in groovy, these rule generate lots of unwanted issue.

Comment with TODO AbcMetric CodeNarc rule : Checks the ABC size metric for
methods/classes. A method (or "closure field") with an ABC score greater than
the maxMethodAbcScore property (60) causes a violation. Likewise, a class that
has an (average method) ABC score greater than the
maxClassAverageMethodAbcScore property (60) causes a violation.

Comment with TODO NestedBlockDepth CodeNarc rule : Checks for blocks or
closures nested more than maxNestedBlockDepth (5) levels deep. This need
discussion and investigation about the impact on groovyScripts

Remove FactoryMethodName codenarc rules : A factory method is a method that
creates objects, and they are typically named either buildFoo(), makeFoo(), or
createFoo(). This rule enforces that only one naming convention is used. It
defaults to makeFoo(), but that can be changed using the property 'regex'.
This kind of control is not suitable in groovy scripting (not OOP)

Comment ImplicitClosureParameter codenarc rules : Checks for the implicit it
closure parameter being used. Also checks if an explicit it parameter has been
specified.  Commented since in certain cases is easy to read. To be discussed.

Comment CyclomaticComplexity codenarc rules : Checks the cyclomatic complexity
for methods/classes.A method (or "closure field") with a cyclomatic complexity
value greater than the maxMethodComplexity property (20) causes a violation.
Likewise, a class that has an (average method) cyclomatic complexityvalue
greater than the maxClassAverageMethodComplexity property (20) causes a
violation.  Commented since some scripts require serious refactoring.

Comment CatchException codenarc rules : Catching Exception is often too broad
or general. It should usually be restricted to framework or infrastructure
code, rather than application code.  Commented since we need to discuss about
execption CatchException for scripting (allow global Exception catching)

Comment ParameterReassignment codenarc rules : Checks for a method or closure
parameter being reassigned to a new value within the body of the
method/closure, which is a confusing and questionable practice. Use a temporary
variable instead.  Commented since we need to discuss about execption
CatchException for scripting (allow global Exception catching)

Remove DuplicateList/MapLiterals codenarc rules : Not sure we want that Code
containing duplicate List literals should contains constants (no class for
groovyScripts)

Remove JavaIoPackageAccess codenarc rules : This rule reports violations of the
Enterprise JavaBeans specification by using the java.io package to access files
or the file system.  Applying this rule imply removing all access to file
system.

Remove instanceof codenarc rules : This rule checks for use of the instanceof
operator. Use the ignoreTypeNames property to configure ignored type names.
Those should be avoided and replaced using polymorphism. In our case, main
issue are about testing Map / List types.

Comment MethodSize codenarc rules : Checks if the size of a method exceeds the
number of lines specified by the maxLines property (100).  This will be
implemented later, such needs method extraction.

Comment ParameterCount codenarc rules : Checks if the number of parameters in
method/constructor exceeds the number of parameters specified by the
maxParameters property.

Remove codenarc ClassJavadoc rules : Makes sure each class and interface
definition is preceded by javadoc. Enum definitions are not checked, due to
strange behavior in the Groovy AST.
@gilPts
Add commented text report type for codenarc
8bb7dec
@gilPts
Fix codenarc AssignmentInConditional rule (OFBIZ-11167)
6752379 
An assignment operator (=) was used in a conditional test. This is
usually a typo, and the comparison operator (==) was intended.

Not Tested in this case.
@gilPts
Fix codenarc ConstantAssertExpression rules : Checks for assert state…
893bcb0 
…ments where the assert boolean condition expression is a constant or literal value.
@gilPts
Fix codenarc DeadCode rule : Dead code appears after a return stateme…
d576d90 
…nt or an exception is thrown. If code appears after one of these statements then it will never be executed and can be safely deleted.
@gilPts
Fix codenarc EmptyCatchBlock rule : In most cases, exceptions should …
1d94a0e 
…not be caught and ignored (swallowed).
@gilPts
Fix codenarc EmptyElseBlock rule : Empty else blocks are confusing an…
2451a4e 
…d serve no purpose.
@gilPts
Fix codenarc EmptyIfBlock rule : Empty if blocks are confusing and se…
151cc1f 
…rve no purpose.
@gilPts
Apply suggestion from Nicolas in pull request
44fd596 
apache#517 (comment)
@gilPts
Remove codenarc disable comment, since using with method on object al…
237c605 
…low multi referencing the same object.

Fix codenarc UnnecessaryObjectReferences rule : 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.
@gilPts
Apply suggestion from Jacques in
54a7f8a 
apache#517 (comment)
@sonarcloud
Copy link

sonarcloud bot commented Apr 5, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug C 1 Bug
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

No Coverage information No Coverage information
2.1% 2.1% Duplication

@danwatford
Copy link
Contributor

danwatford commented Apr 5, 2023 •
edited

@gilPts, It looks like we are getting close to completing this review :)

For the conflicting files, BalanceSheet.groovy has changed quite significantly since the CodeNarc Integration work began. Would it be worth copying the latest BalanceSheet.groovy from trunk and then applying the CodeNarc cleanup.

I'm not sure what would be the cleanest way to do this and keep git happy? Any Git experts on hand to comment?

Similarly, the bug raised by SonarCloud was resolved in trunk for file, themes/helveticus/webapp/helveticus/helveticus-main-theme.less. Bringing that file into your PR would keep SonarCloud happy.

@danwatford
Copy link
Contributor

@nmalin - are you happy with the response to your questions in the review tracker, here - https://cwiki.apache.org/confluence/display/OFBIZ/Codenarc+integration+review+tracker ?

If so, please could you mark the review results for MarketingCampaignReport.groovy, TrackingCodeReport.groovy and LookupBulkAddProducts.groovy as passed.

@danwatford
Copy link
Contributor

@JacquesLeRoux , in the tracker (https://cwiki.apache.org/confluence/display/OFBIZ/Codenarc+integration+review+tracker) you mentioned work was needed.

What sort of changes are needed, or was the WORK_NEEDED review result put there to highlight a possible conflict?

@JacquesLeRoux
Copy link
Contributor

@danwatford , it's possible conflicts related to change for https://issues.apache.org/jira/browse/OFBIZ-12793 "Cannot add a new geo point to a facility"

@danwatford
Copy link
Contributor

@danwatford , it's possible conflicts related to change for https://issues.apache.org/jira/browse/OFBIZ-12793 "Cannot add a new geo point to a facility"

Good news indeed then since, assuming Nicolas' questions have been resolved, that means the only outstanding things left on this PR is to resolve the merge conflicts, which includes FacilityGeoServices.groovy.

@JacquesLeRoux I think it is ok to mark FacilityGeoServices.groovy as PASSED since merge conflicts affect more that just that single file. Or alternatively we should mark all the conflicted files as NEEDS_WORK to highlight that some action still remains to be done?

@JacquesLeRoux
Copy link
Contributor

I changed to "passed", since indeed anyway conflicts will show and will need to be fixed.

@danwatford
Copy link
Contributor

Hi @gilPts ,

I just tried a ./gradlew build with your branch and received a build failure:

> Task :codenarcGroovyScripts FAILED

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':codenarcGroovyScripts'.
> CodeNarc rule violations were found. See the report at: file:///C:/dev/floss/ofbiz/ofbiz-framework/build/reports/codenarc/groovyScripts.html

In the CodeNarc report I see there are 448 Priority 2 issues and 4396 Priority 3 issues listed.

It seems unlikely that we will be able to address all of these quickly.

Do you know if CodeNarc can be configured with a threshold of permitted Priority 2 and Priority 3 issues that would allow the build to proceed? This is approach we use with Checkstyle.

If so, then over time we could gradually reduce the permitted number of issues until we hopefully reach zero, improving code 'hygiene' over time, but also getting CodeNarc integration in place as soon as possible.

@gilPts
Copy link
Contributor Author

gilPts commented Apr 6, 2023

Hello,
Strange, i will check on my local to see what is happening... FYI i did not have plugins installed in that first step. That might explain why the build is success for me...

@gilPts
Copy link
Contributor Author

gilPts commented Apr 6, 2023

About BalanceSheet.groovy and others framework groovy file, I can rebase this branch onto trunk and process the files that are detected by codenarc, then push in another pull request listing the files I worked on.
That will let us review those quickly and finish that work.

@danwatford
Copy link
Contributor

Hello, Strange, i will check on my local to see what is happening... FYI i did not have plugins installed in that first step. That might explain why the build is success for me...

Ah, I think the plugins explains it. The first file reported is plugins/scrum/groovyScripts/AddProductBacklogItem.groovy.

Unless we have a way to exclude, plugins will end up getting included in checks. Having a threshold might be really useful for integrators who have written their own groovy scripts but don't want to or can't reach zero CodeNarc issues at this time.

@danwatford
Copy link
Contributor

About BalanceSheet.groovy and others framework groovy file, I can rebase this branch onto trunk and process the files that are detected by codenarc, then push in another pull request listing the files I worked on. That will let us review those quickly and finish that work.

Sounds good to me

@gilPts
Copy link
Contributor Author

gilPts commented Apr 6, 2023

I did not found yet a way to exclude plugins, if someone has a guess, it could be nice :)
I'll check to have threshold configuration for plugins.

@JacquesLeRoux
Copy link
Contributor

If we suppose it's (only) about integration (GH actions and BB) we could not load the plugins when checking?

@danwatford
Copy link
Contributor

If we suppose it's (only) about integration (GH actions and BB) we could not load the plugins when checking?

I'm not sure. Even if we build ofbiz-framework without the plugins initially. When we subsequently pull the plugins, the 'check' gradle tasks will run again. It would also make the build process more complicated.

I think easiest will be to have a threshold as this will give integrators a way to allow their own plugins/modifications to pass checking without forcing them to immediately fix every issue that CodeNarc might report.

@JacquesLeRoux
Copy link
Contributor

I was actually wondering if Codenarc was providing threshold, according to* it is.

@danwatford
Copy link
Contributor

I was actually wondering if Codenarc was providing threshold, according to* it is.

The Gradle CodeNarc plugin has similar settings to set thresholds:
https://docs.gradle.org/current/dsl/org.gradle.api.plugins.quality.CodeNarcExtension.html

gilPts added a commit to gilPts/ofbiz-framework that referenced this pull request Apr 7, 2023
@gilPts
Apply suggestion from Nicolas in pull request
c431f0e 
apache#517 (comment)
gilPts added a commit to gilPts/ofbiz-framework that referenced this pull request Apr 7, 2023
@gilPts
Apply suggestion from Jacques in
c130b83 
apache#517 (comment)
@gilPts
Copy link
Contributor Author

gilPts commented Apr 7, 2023

I just pushed #618 for the last reviews.
That should be faster than the first one :o)

@danwatford danwatford mentioned this pull request Apr 9, 2023
Merged
@danwatford
Copy link
Contributor

Closing as a rebased version of this PR's branch was handled through #618 .

@danwatford danwatford closed this Apr 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JacquesLeRoux JacquesLeRoux JacquesLeRoux approved these changes

@danwatford danwatford danwatford requested changes

@nmalin nmalin nmalin left review comments

@MkLeila MkLeila MkLeila left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@gilPts @JacquesLeRoux @danwatford @nmalin @MkLeila