Hello everyone, I am proposing a vote for a new rule surrounding PR reviews for the NuttX project.
As you might know, it is possible to "request changes" on GitHub when reviewing a PR. If changes are requested, it is not possible for the PR to be merged until the reviewer who has requested changes dismisses their request or approves the PR. However, an unfortunate ability on GitHub is that anyone with write access (or higher) privileges on the repository can dismiss a reviewer's change request. Unfortunately, this has been used at least twice recently to merge a PR for which reviewers requested changes, which is quite frustrating for the reviewer(s) and does not respect their feedback. I don't think this ability fits with NuttX's ideals of ensuring quality code, and I also think it alienates the reviewers (we have very few of them, so that is bad). My proposal for the rule is as follows: 1. A change-request made by a reviewer can only be "self-dismissed". This indicates the reviewer has been satisfied by the changes made or they have been convinced that their change request is not necessary. 2. If the reviewer is not responsive due to absence (minimum 72 hours), or other reviewers of the PR believe that the reviewer's concern is invalid, a VOTE can be made on the mailing list to overturn the change request. If the VOTE passes, this is the only scenario where the request can be dismissed by someone else. I have included item 2 as a contingency, although I don't expect this scenario to happen often if at all. I suggest that if the vote passes, this rule be included in the contributing guidelines (if there are better locations, please suggest them). I would also just take this opportunity to say: if you have comments regarding changes to a PR, use a change request! The NuttX reviewers are often too nice and only leave comments, or approve a PR but include some feedback in the approval message. Using a change-request prevents the PR from getting accidentally merged while the changes are pending, and that keeps the quality higher :) -- Matteo Golin
signature.asc
Description: PGP signature
