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.

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

[ISSUE 86] upgrade rules matrix with new ids + emojis #88

Merged
merged 14 commits into from
Apr 3, 2023
Merged

Conversation

dedece35
Copy link
Member

@dedece35 dedece35 commented Mar 30, 2023

Hi @utarwyn,
could you check my new version of rule matrix array for JS language please ?
please see https://github.com/green-code-initiative/ecoCode/blob/ISSUE_86/docs/rules/web-matrix.md

@dedece35 dedece35 requested a review from utarwyn March 30, 2023 19:41
@dedece35 dedece35 added 🗒️ documentation Improvements or additions to documentation 🚀 enhancement New feature or request 🔥 in progress 🔥 labels Mar 30, 2023
Copy link
Member

@utarwyn utarwyn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only rule that has been currently reimplemented in the Javascript plugin is "Call a DOM element multiple times without caching". The other rules were in the old version of the linter but have not yet been moved. Maybe we should set them at status 🚧 until they are reintegrated?

This table lists the rules in place in the plugin. The rule names cannot follow the guidelines of Sonar rules, they are based on the ESLint standard.

docs/rules/web-matrix.md Outdated Show resolved Hide resolved
docs/rules/web-rules.md Outdated Show resolved Hide resolved
docs/rules/web-matrix.md Outdated Show resolved Hide resolved
@dedece35
Copy link
Member Author

Hi @utarwyn,
for rule Call a DOM element multiple times without caching, could you give an ID like others ? "EC..."

@utarwyn
Copy link
Member

utarwyn commented Mar 31, 2023

@dedece35
Hi @utarwyn, for rule Call a DOM element multiple times without caching, could you give an ID like others ? "EC..."

As described in my comment #88 (review), javascript rule names are following ESLint standard, so there are using the format: "@ecocode/name". But if you want, we can create a "virtual" ID of the format "EC.." in the table that I can references somewhere else.

@glalloue
Copy link
Contributor

@dedece35
Hi @utarwyn, for rule Call a DOM element multiple times without caching, could you give an ID like others ? "EC..."

As described in my comment #88 (review), javascript rule names are following ESLint standard, so there are using the format: "@ecocode/name". But if you want, we can create a "virtual" ID of the format "EC.." in the table that I can references somewhere else.

@utarwyn : i think we need to set IDs just to get rules references. Per example to set this reference into matrix file, but also to communicate outside the project about one specific rule.
So if we can keep the same format it could be great.
I agree with you that we need to use eslint format because it's the standard format used, but if it's possible i'm ok to integrate your idea to create virtual ID

@glalloue
Copy link
Contributor

@dedece35 : i validate the review, but before to marge, make sure that my modifications are OK too ;)

@dedece35
Copy link
Member Author

dedece35 commented Apr 1, 2023

Hi @glalloue, @utarwyn

I think we should rename the file web-matrix.md to RULES.md as done in ecocode-mobile, no ?

examples here :

another suggestion : move this file to root directory of ecocode plugin to be more accessible

@utarwyn
Copy link
Member

utarwyn commented Apr 1, 2023

I think it's a good idea yes! 👍

@glalloue
Copy link
Contributor

glalloue commented Apr 3, 2023

Hi @glalloue, @utarwyn

I think we should rename the file web-matrix.md to RULES.md as done in ecocode-mobile, no ?

examples here :

another suggestion : move this file to root directory of ecocode plugin to be more accessible

Modifications done

@dedece35
Copy link
Member Author

dedece35 commented Apr 3, 2023

@glalloue, @utarwyn
after a last global check, I found that some rules disappeared : is it ok with that ?

Rules disappeard :

  • Include a CSS file containing directives not used on a page ==> @utarwyn, an idea ?
  • Image tags containing an empty SRC attribute
  • Using strings as arguments to SetTimeout() and setInterval() ==> ok, seen with @utarwyn : useless for JS and not applicable for others languages

I'm waiting for your answers before merging this PR and then create an new 1.1.0 release (for SonarQube marketplace)

@sonarqubecloud
Copy link

sonarqubecloud bot commented Apr 3, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

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
No Duplication information No Duplication information

@utarwyn
Copy link
Member

utarwyn commented Apr 3, 2023

Hello @dedece35, here is my opinion on the different deleted rules:

  • Include a CSS file containing directives not used on a page
    ➡️ this rule is not applicable in the languages that ecoCode supports at the moment (HTML/CSS)
  • Image tags containing an empty SRC attribute
    ➡️ same answer, we need a plugin to parse HTML
  • Using strings as arguments to SetTimeout() and setInterval()
    ➡️ indeed the only applicable language is JavaScript and this rule is already raised by ESLint by default

I think we can go with this version, and rules can be added in the future depending on the plugins we will develop (HTML/CSS for example).

@glalloue
Copy link
Contributor

glalloue commented Apr 3, 2023

@glalloue, @utarwyn after a last global check, I found that some rules disappeared : is it ok with that ?

Rules disappeard :

  • Include a CSS file containing directives not used on a page ==> @utarwyn, an idea ?
  • Image tags containing an empty SRC attribute
  • Using strings as arguments to SetTimeout() and setInterval() ==> ok, seen with @utarwyn : useless for JS and not applicable for others languages

I'm waiting for your answers before merging this PR and then create an new 1.1.0 release (for SonarQube marketplace)

I have voluntarily deleted these rules because they disapeared from the last version of cnumr book (v4) and they were marked as not available in any of our supported languages.
So for me we don't have any value to develop them.

@dedece35 dedece35 merged commit 6e92ae6 into main Apr 3, 2023
@dedece35 dedece35 deleted the ISSUE_86 branch April 3, 2023 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚀 enhancement New feature or request 🗒️ documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants