Thanks for starting the [DISCUSS] thread and for your reply. I'm okay with a week or so delay in case of requested changes in a PR. I just wanted to make sure we were aware of it if we adopt the policy.
I'n supportive of such a policy. Reviewers should honor each other's change requests and not dismiss them, and there is a contingency available (voting on the mailing list) in case a PR gets needlessly stuck. It was mentioned that some reviewers are being "nice" by leaving comments instead of requesting changes. I am one of them :-) Sometimes I do request changes. Other times, I leave a comment without requiring a change. I use a comment when I am offering a suggestion without forcing it. It allows the contributor to use my suggestion, or use an alternative, or just dismiss it. In other words, I leave a comment when I intend for my suggestion to be optional, and I use a change request only when I consider the change to be important enough to block the PR. I think these different possibilities exist for that reason, so I would suggest that reviewers consider using comments for suggestions and change requests for mandatory changes. Also, we should definitely keep the "don't merge your own PR" rule. No change there. :-) Overall I would vote +1 for this policy (or something along these lines). Cheers, Nathan On Tue, Nov 11, 2025 at 1:29 PM Matteo Golin <[email protected]> wrote: > In response to Nathan's message on the previous thread: > https://www.mail-archive.com/[email protected]/msg13877.html > > > I would like to point out that if we have to wait 72 hours, then hold a > > vote lasting 72 more hours, that's 6 days total. Not necessarily the end > of > > the world but could be significant if there's other work in the pipeline > > that depends on the stuck PR. > > Honestly, I think that's fine. If a reviewer has left a change request it > was likely for good reason. I think 72 hours to respond to questions about > their request/new changes is a reasonable time frame, although I'd be > willing to hear other perspectives. I think a 6 day block on a PR with > requested changes is reasonable, there shouldn't be a rush to merge PRs > faster than that if there isn't a consensus of approvals. Critical PRs can > still be merged in less than 6 days provided a reviewer doesn't find issue > with them. > > That being said, I think votes are the ultimate ruler on Apache projects, > so really #2 is more of a guideline. If someone _really_ thinks the PR > needs to be merged ASAP, they can run a vote immediately to request that an > exception to #2 be made and the review dismissed immediately. I don't know > why that would be necessary, but it would only cause a 3-day delay. > > I believe we still have the rule of no self-merging commits, even if you > received enough approvals. This new proposal is in the same spirit of > community approval & concurrence. If someone who was trusted enough by the > project to be a committer has left a binding "request changes" review, that > feedback should be respected and not dismissed until the reviewer has > agreed. > > Best, > Matteo > > On Tue, Nov 11, 2025 at 1:11 PM Matteo Golin <[email protected]> > wrote: > > > Hello everyone, > > > > I am proposing a vote for a new rule surrounding PR reviews for the NuttX > > project. I have started this discussion thread to review the proposal > > first. > > > > Below is my original message: > > > > 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 :) > > > > Best, > > Matteo > > > > >
