Pretty much all my functions have more than 3 returns. SonarQube Bot is going to have agree to disagree on this, spamming every patchset with inline comments is not going to help.
Description
Description
Details
Details
Related Objects
Related Objects
Event Timeline
Comment Actions
I'm not kidding about this, this is very annoying and the number of useful comments I have seen from it is exactly zero. I'll submit a revert for rLTSQcfa9ad30f4d9: Remove inline comment safelist if that is the cause of this.
Comment Actions
@tstarling I deactivated that rule. I'll create a discussion at https://www.mediawiki.org/wiki/Talk:Continuous_integration/Codehealth_Pipeline where we've discussed specific rules in the past.
Or are you saying you want sonarqube bot switched off entirely (for core? for all projects)?
Comment Actions
@tstarling could you add your comments to https://www.mediawiki.org/wiki/Topic:Vpd6k2cu5vchzfo9 please?
Comment Actions
Thanks @kostajh. I think I'd rather not see any non-voting comment as an inline comment. For example, PHPCS failures as inline comments might be a useful feature. But if SonarQube is not voting then I don't want to see inline comments from it. I especially don't want to see them duplicated on every patchset. The problem is that inline comments need to be resolved before the patch can be accepted. Sorting through a mass of bot-generated comments to try to find the human ones that still need resolution is not a task I want to add to my workflow.
Comment Actions
Excessive number of returns is really just an example. On https://gerrit.wikimedia.org/r/c/mediawiki/core/+/608993 SonarQube is putting 8 comments per patchset, of which 3 are about excessive returns. None are actual issues which require any action.
Comment Actions
I especially don't want to see them duplicated on every patchset
I'm not sure there is a great alternative to this. If it were a more serious problem (like an unused variable declaration), and patchset two moved that unused variable into a different method, it would be hard to find what needs to be fixed.
The problem is that inline comments need to be resolved before the patch can be accepted.
Hmm, I agree that this is annoying. The screenshot below shows that Tim is talking about:
In the UI, you can't distinguish between what is a bot comment and what is a human comment. We should file something upstream about this; my understanding is that those should be separated out since they are robot comments and not human comments.
None are actual issues which require any action.
Alternatively, we should go through the rules on the MediaWiki PHP profile and disable all but the ones which really should require action (unused variable, accessing undeclared variable, duplicate code blocks, etc).
And we also need to figure out the process for how specific users can disable a rule for a particular patch (i.e. in sonarcloud.io mark an issue as "won't fix"), which is needed for some analyses like the security related ones. One option is to tie the permissions in our our sonarcloud.io wmftest organization to the wikimedia GitHub organization.
Comment Actions
I also find it fairly irritating that “please fix” is the only reply option for these comments – it seems like whoever wrote that special Gerrit integration didn’t think that the bot could ever be wrong about something. If I want to reply anything else, I first have to click “please fix”, and then edit my own auto-generated comment to say something else.
Comment Actions
I suggest we disable the inline comment behaviour until such a time that a week went by where at least 70% of merged +2'ed mediawiki/* commits have zero would-be inline comments. The idea being that if the bot thinks more than 1/3 commits approved by maintainers made the code worse, I'd like to believe the bot is wrong.
I don't have suggestions at this time for how to get there, but I think it makes for a good framing of success criteria of everything else.
Comment Actions
– it seems like whoever wrote that special Gerrit integration didn’t think that the bot could ever be wrong about something
Yes, we (Code-Health-Metrics group) have thought about and discussed false positives (we meet every two weeks if you are interested to talk, see https://www.mediawiki.org/wiki/Code_Health_Group/projects/Code_Health_Metrics).
The specific message you are writing about ("please fix") comes from Gerrit's implementation of robot comments. Unless I've missed something we cannot override the "Please fix" or "Run details" messages here via RobotCommentInfo, but maybe it's possible to modify a template in our theme. I don't know. Anyway, I think this upstream Gerrit issue partially tracks what we would like to see, which is the ability to say "this is not an issue". There is also a way in sonarcloud.io to mark an issue as "won't fix" or "false positive" so it doesn't show up again for that block of code; but we need consensus that associating the WMF project on sonarcloud.io with the wikimedia GitHub organization is OK in order to manage permissions and I haven't heard any feedback on this idea.
we should go through the rules on the MediaWiki PHP profile and disable all but the ones which really should require action (unused variable, accessing undeclared variable, duplicate code blocks, etc).
@Gehel and I just did this and disabled the ones (for PHP anyway; we didn't do JavaScript) that might be considered controversial. This means you can write 1000 line methods with as many if/else statements as you want now :) Moving forward, we can propose re-enabling specific rules , and if consensus builds, we can re-enable them.
I suggest we disable the inline comment behaviour until such a time that a week went by where at least 70% of merged +2'ed mediawiki/* commits have zero would-be inline comments. The idea being that if the bot thinks more than 1/3 commits approved by maintainers made the code worse, I'd like to believe the bot is wrong.
@Krinkle Thanks for this suggestion. I would propose that we wait a week to see how things look with the narrower subset of rules, if that's OK with everyone.
Comment Actions
@Krinkle Thanks for this suggestion. I would propose that we wait a week to see how things look with the narrower subset of rules, if that's OK with everyone.
Here's a URL we can watch to see what new issues arise since the changes that @Gehel and I made: https://sonarcloud.io/organizations/wmftest/issues?createdAfter=2020-07-03&resolved=false
Comment Actions
There are 286 issues since the changes we made on July 3 (see https://sonarcloud.io/organizations/wmftest/issues?createdAfter=2020-07-03&resolved=false), minus 18 from the VARCHAR2 rule I just disabled (T259447). It looks like a bunch of rule violations are from Vue, I'll see about excluding that directory from analysis.
I'm closing this issue but please reopen (or file a new issue) if there are further specific changes you'd like to see.
Comment Actions
Change 627420 had a related patch set uploaded (by Kosta Harlan; owner: Kosta Harlan):
[integration/config@master] dockerfiles: [java8-sonar-scanner] Excludes resources/lib
Comment Actions
Change 627421 had a related patch set uploaded (by Kosta Harlan; owner: Kosta Harlan):
[integration/config@master] jjb: Update sonar-scanner image
Comment Actions
Change 627420 merged by jenkins-bot:
[integration/config@master] dockerfiles: [java8-sonar-scanner] Excludes resources/lib
Comment Actions
Mentioned in SAL (#wikimedia-releng) [2020-09-16T08:18:46Z] <hashar> Successfully tagged docker-registry.discovery.wmnet/releng/java8-sonar-scanner:0.6.2-s4 # T256942
Comment Actions
Change 627421 merged by jenkins-bot:
[integration/config@master] jjb: Update sonar-scanner image