ofbiz-framework
ofbiz-framework copied to clipboard
apache
Codenarc integration
SonarCloud Quality Gate failed. 
0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells
No Coverage information
9.3% Duplication
![sonarqubecloud[bot] avatar](https://avatars.githubusercontent.com/in/12526?v=4 "Published by a pub.dev verified publisher"){kind=small}
Thanks to Daniel and Gil, you can post your review results at https://cwiki.apache.org/confluence/display/OFBIZ/Codenarc+integration+review+tracker
Hi Gil, Daniel,
Before pushing, would it be possible to have something like https://gitbox.apache.org/repos/asf?p=ofbiz-tools.git;a=blob_plain;f=wiki-files/OFBizJavaFormatter.xml ? See https://cwiki.apache.org/confluence/display/OFBIZ/Coding+Conventions for details
Hi Gil, Daniel,
Before pushing, would it be possible to have something like https://gitbox.apache.org/repos/asf?p=ofbiz-tools.git;a=blob_plain;f=wiki-files/OFBizJavaFormatter.xml ? See https://cwiki.apache.org/confluence/display/OFBIZ/Coding+Conventions for details
Hi @JacquesLeRoux , I would expect this to be possible. I don't use Eclipse, so cannot produce the file myself.
CodeNarc will be policing groovy code style for us to a certain extent, but it would be nice to have our IDEs help us apply a compliant style while authoring code. The configuration files (profiles) could target Eclipse since they can also be imported into IntelliJ.
I would suggest creating a separate ticket to 'create IDE profiles for groovy styling' as we already have a lot do take care of with this ticket.
I would suggest creating a separate ticket to 'create IDE profiles for groovy styling' as we already have a lot do take care of with this ticket.
Done with https://issues.apache.org/jira/browse/OFBIZ-12764
SonarCloud Quality Gate failed.
1 Bug
0 Vulnerabilities
0 Security Hotspots
3 Code Smells
No Coverage information
2.1% Duplication
@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.
@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.
@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?
@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 , 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?
I changed to "passed", since indeed anyway conflicts will show and will need to be fixed.
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.
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...
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.
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.
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
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.
If we suppose it's (only) about integration (GH actions and BB) we could not load the plugins when checking?
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.
I was actually wondering if Codenarc was providing threshold, according to* it is.
- https://stackoverflow.com/questions/27949345/codenarc-priority-meanings
I was actually wondering if Codenarc was providing threshold, according to* it is.
- https://stackoverflow.com/questions/27949345/codenarc-priority-meanings
The Gradle CodeNarc plugin has similar settings to set thresholds: https://docs.gradle.org/current/dsl/org.gradle.api.plugins.quality.CodeNarcExtension.html
I just pushed https://github.com/apache/ofbiz-framework/pull/618 for the last reviews. That should be faster than the first one :o)
Closing as a rebased version of this PR's branch was handled through #618 .