We would like to request addition of the ecoCode plugins to the SonarQube Marketplace.
ecoCode is a collective project aiming to reduce environmental footprint of software at the code level. The goal of the project is to provide a list of static code analyzers to highlight code structures that may have a negative ecological impact: energy and resources over-consumption, “fatware”, shortening terminals’ lifespan, etc.
ecoCode is based on evolving catalogs of good practices, for various technologies.
Looking at the bureaucratic requirements it appears that you’ve mis-read the pattern for plugin keys, which is [a-z0-9]. Those dashes aren’t allowed, in the key, they’re there to describe ranges. So I’ll need you to update your plugin keys. The metadata job simply won’t work like they are.
The other bureaucratic requirements look fine.
The PR is also missing something. Specifically, the part where you register your new files/plugins in the master list in update-center-source.properties.
But before you do that, I’ll need to test the plugins. Can you provide or point me to projects I can/should test with?
I’ve started testing with the Java version of the plugin, using the project you provided (as communicated via GH since I was already commenting in your project).
Thanks very much for providing a project that seems to raise at least one issue on each rule. TBH, I don’t usually give rule-by-rule feedback, but it’s a small set and I was curious to see your rules in action.
Here’s what I saw in my testing.
IMO these things should be handled on your side, but if you don’t want to, we need to talk about it.
For (cnumr-java:S63), the issue message doesn’t match up with the rule title/description:
While I don’t disagree that what you’ve highlighted is bad practice and should be avoided, having an issue and message that don’t match up to the rule will be confusing for end users.
Some of your rules seem to have Snnn ids. While I don’t think there will be a direct clash with our own Snnn rule ids, this still makes me uneasy. Would you consider changing those rule numbers to maybe add a character to the prefix? Or a suffix? Or… something?
Correct the plugin key. Yes, I know you know this already. I’m including it here so I have one place to go back to with all the things to double-check.
You might want to shorten, and perhaps translate your rule repo name:
You may also want to re-proof-read your rules (emphasis mine)
…never get it during iteration, It saves CPU cycles so unless energy consumption.
After looking at only a few of your rules, it strikes me that most (all?) of them are really about performance. I suggest you also tag them with that: performance
At least one Noncompliant Code Example (cnumr-javaS78) doesn’t include a //Noncompliant comment. Without it I, as a user, spend a lot of time going back and forth between the two code samples to figure out what changes & why.
Also on S78, I find the wording slightly odd: “in batch update” vs… “in a loop”?
Do not call a function when declaring a for-type loop in order to avoid function calls each iterations. It saves CPU cycles.
Might want to sharpen this language a bit. Theoretically it’s fine to call a method in the initialization (int i=foo.size()), right?
GRSP0028 - there’s really not enough to go on here. In fact, there’s nothing to go on here. This needs a description.
*GSCIL is a subset of S69. In general you only want one rule to raise an issue on a given piece of code. I suggest dropping this rule.
S79, about using try-with-resources raises an issue on code where, at a glance, it looks like all the resources are properly disposed. Users will perceive this as a false positive.
You haven’t provided a default profile. That’s totally fine since you’re not handling the basic language implementation. And I wonder if it makes sense to create a profile that’s all of your rules and all of the built-in #performance rules…?
My thinking here is not that people would necessarily use your profile as-is, but that perhaps they would copy it into their own profiles & thus ensure that all #performance rules got turned on (and hopefully followed!). This is mostly an idle thought on my side, so feel free to completely ignore it.
As for rule key prefix, we have several “fairly identical” rules like Java/S67 and PHP/S67. Do we need to define different prefixes for each rule repository (one for Java and another for PHP), or do we need to define the same prefix for each rule repository of ecoCode (an identical prefix for the repositories of each of our 6 different ecoCode plugins)?
Also, do we need to define properties (name, description, priority, tags) for each of the rules both in the class file (via the @org.sonar.check.Rule annotation) and in the JSON file? Or is it better to just set the key property of the @org.sonar.check.Rule annotation in the class, and the rest of properties in the JSON file?
In production, the repo key is prefixed onto the rule key, like java:S123. So there’s no need to differentiate rule key prefixes across languages. In fact, it’s better if the keys line up across languages for what’s essentially the same rule.
That’s an implementation detail I’m slightly fuzzy on, but I believe it’s one or the other, with the JSON files being what’s used internally since we can easily generate them from our rule repo.
I really like the energy comparison at the bottom of the PHP rules. This is really nice, and I think it will likely help adoption.
In checking all the plugins’ change logs, I see this:
WARNING : since this plugin version, ids of plugin rules changed. In consequence, if you have already made some issue checks in your SonarQube instance, you will have to do them again (example : false-positive issues will appear again)
There is a mechanism available to migrate rule ids (and even repository names). Using it would mean that all the old issues would be seamlessly migrated to the new rule ids and no manual work, e.g. FP-ing, would be lost. (BTW, I can’t tell you the particulars, but it’s likely visible in the API.)
Before we put these versions into the marketplace, do you want to take a look at that? It’s up to you.
I find it odd that the Java plugin has only Code Smell rules and the PHP and Python plugins have only Bug rules, especially when some of the rules are the same from language to language (e.g. ++i instead of i++)
ecocode-php:EC34’s title is “Avoid using try-catch-finally”, but it raises issues on try-catch blocks without the finally clause. The code sample clears it up, but it seems the rule is really “Avoid using try-catch” or perhaps “Avoid using try-catch and try-catch-finally”?
For ecocode-java:EC63 (I see you’ve re-keyed it from S63. Thank you!) I see that the issue message still doesn’t make sense:
Explicitly, the variable is assigned. I’m guessing it’s not used…?
I’ve mentioned this in the pull request, but
I get an error running the metadata generation job:
The plugin ‘php’ required by ‘ecocodephp’ is missing. Unable to find plugin with key php → [Help 1]
If the other two plugins explicitly “require” their respective language analyzers, we’ll hit this problem with them too. Since the analyzers were rolled more tightly into SonarQube, they no longer appear in the Marketplace. So I need you to update the poms to remove this requirement & issue new releases.