> 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.
I think this is a good use of the regular review comments! I just think a lot of good feedback gets overlooked because it's not "binding". I didn't mean to call anyone out for using regular comments, I acknowledge there's definitely use cases for them! Just sometimes "minor" changes snowball into big inconsistencies. Thanks for your feedback on the proposal :) Matteo On Tue, Nov 11, 2025 at 2:32 PM Nathan Hartman <[email protected]> wrote: > 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 > > > > > > > > >
