1608339 - Review bot comments for older commits are added noise

Status: NEW

(Developer Infrastructure :: Source Code Analysis, task, P3)

Description

[Mike Hommey [:glandium]]

Example of a simple flow of action:

• push a new commit to phabricator, creating a new differential.
• turns out, there are some problems found by clang-format or clang-tidy, or whatever. This creates a comment in phabricator. In good cases, it pinpoints to lines in the code. In worse cases, it's just a general comment.
• you update your code to address those issues and push it to phabricator.

Currently, nothing else happens, so let's look at the reviewer's perspective now, assuming they come after all the above:

• open the phabricator differential
• see there's a comment from reviewbot about some problem found.
• now you have to figure out whether this was addressed or not. In the good cases from above, the lack of inline comments is an indicator that the comment is outdated. But since there are cases where there aren't inline comments at all, one needs to be careful it's one of those cases.

Now imagine a differential where there's been multiple passes with multiple updates, each of them triggering reviewbot messages except maybe the last one because it's finally all clean.

Add to that comments going between the reviewer and the patch author in between those reviewbot comments. The added noise makes it harder to follow conversations.

Could outdated reviewbot comments be removed? If not could reviewbot post a "everything was fixed" message when there were previous failed attempts, making it clearer that the current diff is clean? Or maybe, could reviewbot not use comments to post its status, but maybe use a field like the Test Plan (or even better, if possible, a dedicated field) and update that field, leaving only the last status there?

Comment 4

[Andi [:andi]]

As Sylvestre pointed out here the API provided by Phabricator in order to manage comments it's pretty limited.

Comment 5

[Andi [:andi]]

bastien I don't think we have support on removing comments from Phabricator, maybe we should not post issues as comments?

Comment 6

[Bastien Abadie [:bastien]]

We can't remove comments from Phabricator.
The next release (this week) of code review bot should publish issues as Phabricator Lint results which are tied to a specific build (and diff). So an update will automatically remove all the previously detected issues.

Comment 7

[Sylvestre Ledru [:Sylvestre]]

We should prepare some communication on this change btw