Clean code attributes and better descriptions complying with Sonar Coding rule guidelines #349

pbaumard · 2024-08-09T16:49:52Z

Ensures the Sonar Coding rule guidelines for rule specifications are followed with tests. This should avoid most of the comments made by Sonar reviewers, e.g. in ecocode plugin request or ecocode js plugin request:

Fix titles
Fix description sections. Sonar is very strict about section titles in its EducationRuleLoader
Use standard sustainability tag instead of eco-design since SonarSource is considering adding Sustainability as a standard software quality
Add Clean code attributes for all rules.
Improved description wording with shorter sentences.
Avoid mardown in asciidoc files to ease the use of asciidoc editors

It will improve the user experience when using Creedengo plugins. All the differences from the standard Sonar rules are still stricking me when I use the rules: the wrong Clean Code attributes, the weird rule titles format, non standard sections in descriptions, etc.
This may give a bad first impression to many Creedengo users and may discourage the use of Creedengo plugins.
SonarQube is a code quality solution so its users typically care a lot about consistency.

To be honest I am quite suprised that Sonar does not ensure that the Community plugins comply with the Sonar Coding rule guidelines. Maybe plugins will have to comply in the future.

dedece35 · 2024-08-11T21:32:22Z

pbaumard

Hi @pbaumard ,

thank you for this PR ... very interesting but a lot of modifications.
Can you give us your reference documentation to prove or support your proposition ?

Furthermore, your build is not ok on github action ... please make a correction for it.
In my local machine (Mac OS), when I launch your unit test, I have a different error :

[INFO] Running fr.greencodeinitiative.RulesTest
[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.506 s <<< FAILURE! -- in fr.greencodeinitiative.RulesTest
[ERROR] fr.greencodeinitiative.RulesTest.testRules -- Time elapsed: 0.018 s <<< FAILURE!
org.opentest4j.AssertionFailedError: array contents differ at index [0], expected: <csharp> but was: <python>

please check it in your local machine.

Last question : why is your PR in Draft mode ? Do you still want to work on it ?

pbaumard · 2024-09-04T08:09:18Z

This PR is now ready for review.

pbaumard · 2024-09-24T08:09:39Z

@dedece35 Any chance to get this PR reviewed?

It would then be necessary to update all plugins.
Android plugin would neeed to migrate to ecoCode rule specifications: green-code-initiative/ecoCode-android#79

dedece35 · 2024-09-24T13:24:12Z

@dedece35 Any chance to get this PR reviewed?

It would then be necessary to update all plugins. Android plugin would neeed to migrate to ecoCode rule specifications: green-code-initiative/ecoCode-android#79

hello ... thank you for the work ... review in progress !

utarwyn

Thank you for this huge work!
I did a quick tour on HTML/JavaScript rules changes, it seems good overall 👍

But I'm wondering if this PR isn't too large... it's really huge and I can't see myself going through all the changes without spending half a day on it..

utarwyn · 2024-09-24T13:43:46Z

ecocode-rules-specifications/src/main/rules/EC31/javascript/EC31.asciidoc

+----
+
+=== Compliant solution
+
 We recommend using the following formats:


Why this section is now in "Compliant solution"?

The section change was reverted.

utarwyn · 2024-09-24T13:46:06Z

ecocode-rules-specifications/src/main/rules/EC12/EC12.json

  "compatibleLanguages": [
    "JAVASCRIPT",
    "TYPESCRIPT"
  ]
-}
+}


There must be an empty line at the end of the file

@utarwyn, why is an empty line mandatory ? if not present, the system doesn't work ?
if yes, @pbaumard, please make the correction for all empty lines deleted at the end of this kind of files.

I added the spotless plugin to check newlines for all json files. This is just a matter of consistency.

utarwyn · 2024-09-24T13:48:26Z

ecocode-rules-specifications/src/main/rules/EC36/html/EC36.asciidoc

@@ -11,25 +11,33 @@ However, even without autoplay, segments of video or audio files might still dow
 This leads to unnecessary data usage, especially if users don't commence playback.
 To mitigate this, it's crucial to prevent browsers from preloading any content by configuring the appropriate settings.

-Video:
+== How to fix it in video


I think both section names are wrong here, we do not provide a video to explain how to fix the problem

Suggested change

== How to fix it in video

== How to fix a video element

Those section titles were changed in latests commits.

utarwyn · 2024-09-24T13:50:11Z

ecocode-rules-specifications/src/main/rules/EC523/javascript/EC523.asciidoc

@@ -63,6 +69,11 @@ Location.requestPermissionsAsync(); // Compliant

 == Resources

+=== Best practises
+
+- https://github.com/cnumr/best-practices-mobile[Mobile-specific Best Practices] by https://collectif.greenit.fr/index_en.html[CNumR]


Do you have a more precise source?

This is the same source as the java version of EC523. Do you have a better source?

dedece35 · 2024-09-25T08:54:07Z

HI @zippy1978 and @olegoaer,
could you please make a review on all modifications done for EC5XX et and EC6XX, please ?

dedece35 · 2024-09-24T14:05:39Z

ecocode-rules-specifications/src/main/rules/EC523/EC523.json

@@ -1,11 +1,16 @@
 {
-  "title": "Sobriety: Thrifty Geolocation (minTime)",


why do you delete the title ? maybe refactor it like done in another updates of this PR, no ?

The title seems useless since it is redefined in the JSON of all EC523 subdirectories, so this title would always be overriden, right?

dedece35 · 2024-09-24T14:06:18Z

ecocode-rules-specifications/src/main/rules/EC523/java/EC523.json

  "tags": [
-    "sobriety",


why do you delete "sobriety" ?

At first I thought sobriety was too close to existing eco-design or performance tags. I later changed my mind to include all tags from https://github.com/cnumr/best-practices-mobile

sobriety tag removal was reverted in latests commits.

dedece35 · 2024-09-24T14:07:47Z

ecocode-rules-specifications/src/main/rules/EC523/javascript/EC523.json

  "tags": [
-    "sobriety",


why do you delete "sobriety" tag on several updates ?

See above reply in #349 (comment)

dedece35 · 2024-09-25T06:17:51Z

ecocode-rules-specifications/src/main/rules/EC2/java/EC2.asciidoc


-SWITCH statement solution + refactor solution
+With a `switch`:


here, please add the explanation like done in python and PHP files

Done in latest commits.

dedece35 · 2024-09-25T08:47:08Z

ecocode-rules-specifications/src/main/rules/EC85/csharp/EC85.asciidoc

+
+== Resources
+
+Reference/Validation is unknown.


maybe move above link to here

Done in latest commits.

dedece35 · 2024-09-25T08:48:18Z

ecocode-rules-specifications/src/main/rules/EC2/java/EC2.asciidoc


-SWITCH statement solution + refactor solution
+With a `switch`:


here, please add the explanation like done in python and PHP files

Done in latest commits.

dedece35 · 2024-09-25T08:48:51Z

ecocode-rules-specifications/src/main/rules/EC69/EC69.json

@@ -1,5 +1,5 @@
 {
-  "title": "Do not call a function when declaring a for-type loop",
+  "title": "Functions should not be called when declaring a for-type loop",


please update title description to be clearer : please, replace “when declaring a for-loop type” by “inside a for-loop condition”

Done in latest commit.

dedece35 · 2024-09-25T08:52:08Z

ecocode-rules-specifications/src/main/rules/EC12/EC12.json

  "compatibleLanguages": [
    "JAVASCRIPT",
    "TYPESCRIPT"
  ]
-}
+}


@utarwyn, why is an empty line mandatory ? if not present, the system doesn't work ?
if yes, @pbaumard, please make the correction for all empty lines deleted at the end of this kind of files.

dedece35 · 2024-09-25T14:14:46Z

pom.xml

@@ -94,13 +94,56 @@
    </issueManagement>

    <properties>
+        <java.version>11</java.version>


all these pom.xml modifications must be in the ecocode-rules-specifications pom.x instead and the current parent pom.xml

all other plugins work with java 17. why do you put java 11 ?

POM changes have been moved to ecocode-rules-specifications.

Some of the configuration, including java 11 is coming from
https://github.com/green-code-initiative/ecoCode-java/blob/main/pom.xml

Java version could be changed.

dedece35 · 2024-09-25T14:17:30Z

ecocode-rules-specifications/src/test/java/fr/greencodeinitiative/RulesTest.java

@@ -0,0 +1,263 @@
+/*


I'm ok to create a new Test class to check all files and all rules. but I think we can do it simplier, maybe.
I will run your test class and give my feedback soon.

@dedece35 Did you have time to review the test class?

dedece35 · 2024-09-25T14:19:55Z

ecocode-rules-specifications/src/main/rules/EC1/EC1.json

@@ -3,15 +3,10 @@
  "type": "CODE_SMELL",
  "status": "ready",
  "remediation": {
-    "func": "Constant\/Issue",


did you test your modification in a local SonarQube to check if the description is well displayed ?

Those func attribute changes were reverted.

…ion-normalization

pbaumard · 2024-10-24T09:18:51Z

review in progress !

@dedece35 Any progress on the review?

I don't see any point waiting for me.
Please remove the label "waiting commiter" to clarify this PR status.

…ion-normalization

pbaumard · 2024-12-19T13:34:28Z

@dedece35 It seems all the PR builds are failing because of a "401 Unauthorized" error on the Sonar server:

Error: Failed to execute goal org.sonarsource.scanner.maven:sonar-maven-plugin:5.0.0.4389:sonar (default-cli) on project creedengo-rules-specifications: Error status returned by url [https://api.sonarcloud.io/analysis/jres?os=linux&arch=x86_64]: 401

Note that PRs are also failing for the same reason on other repositories, e.g. on creedengo-java:
https://github.com/green-code-initiative/creedengo-java/actions/runs/12127895160/job/33813144041

…ion-normalization

dedece35 · 2025-01-03T21:33:58Z

pom.xml

@@ -1,470 +1,594 @@
 <?xml version="1.0" encoding="UTF-8"?>
 <project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 https://maven.apache.org/xsd/maven-4.0.0.xsd">
-    <modelVersion>4.0.0</modelVersion>
+  <modelVersion>4.0.0</modelVersion>


sorry, but why do you want to modify XML formatting ? we use default XML formatting from IntelliJ : tabulation = 4 spaces

dedece35 · 2025-01-03T21:34:57Z

pom.xml

+  <scm>
+    <connection>scm:git:https://github.com/green-code-initiative/creedengo-rules-specifications</connection>
+    <developerConnection>scm:git:https://github.com/green-code-initiative/creedengo-rules-specifications</developerConnection>
+    <tag>HEAD</tag>


why do you want these minor modifications ?

dedece35 · 2025-01-03T21:37:09Z

pom.xml

-        -->
-        <revision>current-SNAPSHOT</revision>
+  <properties>
+    <!-- Default version when disabling Maven `maven-git-versioning-extension`


why not ... but if you do these kind of modifications, we will have a lot of modifications ... I remain doubtful about these modifications.
for me, it's more readable with old comment mode.

dedece35 · 2025-01-03T21:47:02Z

pom.xml

+        <executions>
+          <execution>
+            <id>convert-to-html</id>
+            <goals>


why do you want to change the order ?

dedece35 · 2025-01-03T21:49:48Z

pom.xml

+        <artifactId>maven-assembly-plugin</artifactId>
+        <version>3.7.1</version>
+        <configuration>
+          <appendAssemblyId>true</appendAssemblyId>


why this move ?

dedece35 · 2025-01-03T21:53:57Z

pom.xml

+        </plugins>
+      </build>
+    </profile>
+  </profiles>
 </project>


sorry, but pom.xml modifications are too important and disparate ...
for me, first, please make minimum modifications (adding dependencies test for example)
and secondly (in another PR, formating modifications ... on which we will discuss about veracity)

This project was not using any Java code previously so there will be anyway many changes in the POM.

The formatting changes related to Spotless sortPom are very small compared to changes related to the introduction of Java code.

dedece35 · 2025-01-03T21:55:48Z

src/main/rules/GCI1/GCI1.json

@@ -1,5 +1,5 @@
 {
-  "title": "Avoid Spring repository call in loop or stream operations",
+  "title": "Spring repository should not be called in loop or stream operations",


in my point of view, I prefer a turn of phrase without negation, thus, I prefer "avoid" than "should not be" ... but I can ask for the point of view of core team

From Sonar Coding rule guidelines:

The title of the rule should match the pattern "X should [ not ] Y" for most rules. Note that the "should [ not ]" pattern is too strong for Finding rules, which are about observations on the code. Finding titles should be neutral, such as "Track x".

And as a developer I like the "X should [ not ] Y" pattern, because the expected fix is clear.
With "Avoid X" pattern sometimes it is not clear whether the code is avoiding X and it should not or the opposite, that is the code is using X and it should avoid it.

dedece35 · 2025-01-03T22:11:33Z

Hi @pbaumard ,

as @utarwyn, sorry, but for me also, this PR is so huge ...
I have just added some comments on pom.xml ... I'm not ok with formatting modifications for example.

Sorry, to say it, but for me, I can't accept this PR with all these modifications ... mainly because there too important modifications on many files and I have so many questions / review feedbacks.
As we can see in your top description of current PR, this one deals with 5 points ... and as best practice, we should create one PR per point : like this, we can discuss more accurately inside each PR.

What do you think about it @utarwyn , @olegoaer , @glalloue, @zippy1978 , @Djoums ?

Djoums · 2025-01-03T22:31:22Z

Hi @pbaumard ,

as @utarwyn, sorry, but for me also, this PR is so huge ... I have just added some comments on pom.xml ... I'm not ok with formatting modifications for example.

Sorry, to say it, but for me, I can't accept this PR with all these modifications ... mainly because there too important modifications on many files and I have so many questions / review feedbacks. As we can see in your top description of current PR, this one deals with 5 points ... and as best practice, we should create one PR per point : like this, we can discuss more accurately inside each PR.

What do you think about it @utarwyn , @olegoaer , @glalloue, @zippy1978 , @Djoums ?

I agree this PR should not be commited as is. As I can see there's a mix of issues :

Too many changed files at once, reviews become shallow in those cases
Some changes may require discussion, for ex the title wording style
Some changes come for the author's preference/settings

I'd split this PR by files and/or by change types (titles, tags, layout, etc).

pbaumard · 2025-01-06T12:12:50Z

* Too many changed files at once, reviews become shallow in those cases

The many files change are mainly just a consequence of the testing added which make the JSON complying with Sonar Coding rule guidelines. It would be quite artificial to limit the tests to a subset of files. And it would be also artificial to create many PRs all modifying many files just adre

* Some changes may require discussion, for ex the title wording style

Title wording style is coming from Sonar Coding rule guidelines

* Some changes come for the author's preference/settings

Formatting changes have been reverted and are addressed in #366

* Too many changed files at once, reviews become shallow in those cases

The many files change are mainly just a consequence of the testing added which make the JSON complying with Sonar Coding rule guidelines. It would be quite artificial to limit the tests a a subset of files.

* Some changes may require discussion, for ex the title wording style

Title wording style is coming from Sonar Coding rule guidelines

I'd split this PR by files and/or by change types (titles, tags, layout, etc).

Layout split is done in #366

Splitting by file is hard since the tests are applied to all files.

If this is split by change type, the number of files in PRs will not change.

A split would require even more work and would not limit the amount of review required.

If there is one area you think should be excluded from this PR, I could try to split it.

This PR is really just a test-driven PR applying the Sonar Coding rule guidelines.
I edited this PR description above to explain why I think this PR is important.

Rule specifications normalization in progress

c3ca195

dedece35 assigned pbaumard Aug 11, 2024

dedece35 added 🗒️ documentation Improvements or additions to documentation 🚀 enhancement New feature or request 🏗️ refactoring refactoring for best practices 🔥 in progress 🔥 👀 👀 waiting commiter 👀 👀 labels Aug 11, 2024

pbaumard added 4 commits September 4, 2024 09:28

Tests are now OK

bd8a4f1

Add tests for title length and prefix

706c181

Add java version

06e0173

Fix test unsing unsorted list

dabb897

pbaumard marked this pull request as ready for review September 4, 2024 08:01

utarwyn reviewed Sep 24, 2024

View reviewed changes

dedece35 requested review from dedece35, zippy1978 and olegoaer September 25, 2024 08:53

pbaumard added 3 commits September 25, 2024 15:23

Add license-maven-plugin and spotless-maven-plugin

a0a2c2e

Revert section change for EC31

f23f81d

Better section titles in EC36

613a418

dedece35 requested changes Sep 25, 2024

View reviewed changes

pbaumard added 4 commits September 25, 2024 18:06

Revert tag changes

3975cd0

Consistent descriptions for EC2

7d53fc2

Move pom changes to ecocode-rules-specifications

0fa8fc7

Move link to resources

a3add9e

pbaumard added 2 commits September 25, 2024 18:50

Typography

52c0d78

Better EC69 title

abfcfea

pbaumard requested review from dedece35 and utarwyn September 26, 2024 06:57

pbaumard added 4 commits September 26, 2024 09:49

Style

94c42dc

Test refactoring

8470f60

Merge remote-tracking branch 'origin/main' into feature/rule-descript…

f714c48

…ion-normalization

Merge remote-tracking branch 'origin/main' into feature/rule-descript…

7b43fe8

…ion-normalization

Merge branch 'main' into feature/rule-description-normalization

ffa58e8

dedece35 removed the 👀 👀 waiting commiter 👀 👀 label Nov 18, 2024

pbaumard added 2 commits November 20, 2024 10:17

Use standard sustainability tag instead of eco-design

ee7f507

Merge remote-tracking branch 'origin/main' into feature/rule-descript…

f2f8eff

…ion-normalization

pbaumard and others added 2 commits January 2, 2025 14:19

Merge remote-tracking branch 'origin/main' into feature/rule-descript…

3d2e6c8

…ion-normalization

Merge branch 'main' into feature/rule-description-normalization

4973db6

dedece35 requested changes Jan 3, 2025

View reviewed changes

Merge branch 'main' into feature/rule-description-normalization

d6dbc50

Merge branch 'main' into feature/rule-description-normalization

14b4072

Fix GCI82 description

7fc9af0

pbaumard changed the title ~~Rule specifications normalization~~ Complying with Sonar Coding rule guidelines Jan 7, 2025

dedece35 marked this pull request as draft January 12, 2025 21:48

Revert pom formatting changes

3d041ce

pbaumard changed the title ~~Complying with Sonar Coding rule guidelines~~ Clean code attributes and better descriptions complying with Sonar Coding rule guidelines Jan 13, 2025

pbaumard marked this pull request as ready for review January 13, 2025 12:22

		@@ -1,11 +1,16 @@
		{
		"title": "Sobriety: Thrifty Geolocation (minTime)",


		SWITCH statement solution + refactor solution
		With a `switch`:

Clean code attributes and better descriptions complying with Sonar Coding rule guidelines #349

Are you sure you want to change the base?

Clean code attributes and better descriptions complying with Sonar Coding rule guidelines #349

Conversation

pbaumard commented Aug 9, 2024 • edited Loading

dedece35 commented Aug 11, 2024 • edited Loading

pbaumard commented Sep 4, 2024

pbaumard commented Sep 24, 2024

dedece35 commented Sep 24, 2024

utarwyn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pbaumard Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dedece35 commented Sep 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pbaumard commented Oct 24, 2024

pbaumard commented Dec 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pbaumard Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

dedece35 commented Jan 3, 2025

Djoums commented Jan 3, 2025

pbaumard commented Jan 6, 2025 • edited Loading

pbaumard commented Aug 9, 2024 •

edited

Loading

dedece35 commented Aug 11, 2024 •

edited

Loading

pbaumard Sep 25, 2024 •

edited

Loading

pbaumard Jan 6, 2025 •

edited

Loading

pbaumard commented Jan 6, 2025 •

edited

Loading