I think it is always better to just add a comment instead of a Change Request, but sometimes I put the a comment asking the change, but then two people approve the PR, in this case I have no other option than add a Change Request.
I think this proposal is good to avoid ignoring the CR. Currently to merge a PR all comments need to be resolved. But just like Change Request anyone with write access can mark comments as Resolved. So Change Request is the last barrier. BR, Alan On Tuesday, November 11, 2025, Matteo Golin <[email protected]> wrote: > > 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 > > > > > > > > > > > > > >
