You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@nuttx.apache.org by Matteo Golin <ma...@gmail.com> on 2025/11/11 16:42:29 UTC
[VOTE] Dismissing PR reviews - rules
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
Re: [VOTE] Dismissing PR reviews - rules
Posted by Gregory Nutt <sp...@gmail.com>.
Under Apache, nothing is really enforceable. No one person or group has any power and cannot compel anyone to do anything. Freedom is a core Apache value.
________________________________
From: Sebastien Lorquet <se...@lorquet.fr>
Sent: Wednesday, November 12, 2025 3:05 AM
To: dev@nuttx.apache.org <de...@nuttx.apache.org>
Subject: Re: [VOTE] Dismissing PR reviews - rules
At this point, it sounds like it would be desirable to have enforceable
consequences for people that do not follow the rules.
Sebastien
On 11/12/25 04:36, Matteo Golin wrote:
> Hm, I guess I just assumed this wasn't the case since it was being ignored.
> I don't think this vote is really necessary if we already have this rule in
> the guidelines. I guess the vote adds stricter wording, i.e. it is only up
> to the person who started the discussion to decide if it's resolved, no one
> else.
>
> Thanks for pointing that out!
>
> Matteo
>
> On Tue, Nov 11, 2025, 9:30 PM Tomek CEDRO <to...@cedro.info> wrote:
>
>> +1 in general for the idea :-)
>>
>> We already have this rule in Contributing Guidelines in 1.17.3 :-)
>>
>> https://github.com/apache/nuttx/blob/master/CONTRIBUTING.md#117-merge-rules
>>
>> 1.17.3. Merge of PRs with unresolved discussions and "change request"
>> marks is not allowed.
>>
>> Another story if contributors and maintainers adhere to the CG ;-)
>>
>> Tomek
>>
>> On Tue, Nov 11, 2025 at 5:43 PM Matteo Golin <ma...@gmail.com>
>> wrote:
>>> 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
>>
>>
>> --
>> CeDeROM, SQ7MHZ, http://www.tomek.cedro.info
>>
Re: [VOTE] Dismissing PR reviews - rules
Posted by Sebastien Lorquet <se...@lorquet.fr>.
At this point, it sounds like it would be desirable to have enforceable
consequences for people that do not follow the rules.
Sebastien
On 11/12/25 04:36, Matteo Golin wrote:
> Hm, I guess I just assumed this wasn't the case since it was being ignored.
> I don't think this vote is really necessary if we already have this rule in
> the guidelines. I guess the vote adds stricter wording, i.e. it is only up
> to the person who started the discussion to decide if it's resolved, no one
> else.
>
> Thanks for pointing that out!
>
> Matteo
>
> On Tue, Nov 11, 2025, 9:30 PM Tomek CEDRO <to...@cedro.info> wrote:
>
>> +1 in general for the idea :-)
>>
>> We already have this rule in Contributing Guidelines in 1.17.3 :-)
>>
>> https://github.com/apache/nuttx/blob/master/CONTRIBUTING.md#117-merge-rules
>>
>> 1.17.3. Merge of PRs with unresolved discussions and "change request"
>> marks is not allowed.
>>
>> Another story if contributors and maintainers adhere to the CG ;-)
>>
>> Tomek
>>
>> On Tue, Nov 11, 2025 at 5:43 PM Matteo Golin <ma...@gmail.com>
>> wrote:
>>> 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
>>
>>
>> --
>> CeDeROM, SQ7MHZ, http://www.tomek.cedro.info
>>
Re: [VOTE] Dismissing PR reviews - rules
Posted by Matteo Golin <ma...@gmail.com>.
Hm, I guess I just assumed this wasn't the case since it was being ignored.
I don't think this vote is really necessary if we already have this rule in
the guidelines. I guess the vote adds stricter wording, i.e. it is only up
to the person who started the discussion to decide if it's resolved, no one
else.
Thanks for pointing that out!
Matteo
On Tue, Nov 11, 2025, 9:30 PM Tomek CEDRO <to...@cedro.info> wrote:
> +1 in general for the idea :-)
>
> We already have this rule in Contributing Guidelines in 1.17.3 :-)
>
> https://github.com/apache/nuttx/blob/master/CONTRIBUTING.md#117-merge-rules
>
> 1.17.3. Merge of PRs with unresolved discussions and "change request"
> marks is not allowed.
>
> Another story if contributors and maintainers adhere to the CG ;-)
>
> Tomek
>
> On Tue, Nov 11, 2025 at 5:43 PM Matteo Golin <ma...@gmail.com>
> wrote:
> >
> > 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
>
>
>
> --
> CeDeROM, SQ7MHZ, http://www.tomek.cedro.info
>
Re: [VOTE] Dismissing PR reviews - rules
Posted by Tomek CEDRO <to...@cedro.info>.
+1 in general for the idea :-)
We already have this rule in Contributing Guidelines in 1.17.3 :-)
https://github.com/apache/nuttx/blob/master/CONTRIBUTING.md#117-merge-rules
1.17.3. Merge of PRs with unresolved discussions and "change request"
marks is not allowed.
Another story if contributors and maintainers adhere to the CG ;-)
Tomek
On Tue, Nov 11, 2025 at 5:43 PM Matteo Golin <ma...@gmail.com> wrote:
>
> 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
--
CeDeROM, SQ7MHZ, http://www.tomek.cedro.info
Re: [VOTE] Dismissing PR reviews - rules
Posted by Matteo Golin <ma...@gmail.com>.
You're right, a DISCUSS thread would be a good idea! I suppose discussion
shouldn't take place in a VOTE thread. I'll start one.
Best,
Matteo
On Tue, Nov 11, 2025 at 1:02 PM Nathan Hartman <ha...@gmail.com>
wrote:
> Hi,
>
> It is important to have a contingency in case a reviewer requests changes
> and then stops participating for whatever reason (for example, gets busy
> with other work in the meantime). So, that's good thinking to include item
> #2.
>
> 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.
>
> Are you sure you'd like to go straight to a [VOTE] without a [DISCUSS]
> thread first? Perhaps others may propose alternative contingency ideas, or
> offer thoughts on why reviewers should be able to dismiss other reviewers'
> change requests, so we can consider all aspects of this before we vote.
>
> Cheers,
> Nathan
>
> On Tue, Nov 11, 2025 at 11:43 AM Matteo Golin <ma...@gmail.com>
> wrote:
>
> > 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
> >
>
Re: [VOTE] Dismissing PR reviews - rules
Posted by Nathan Hartman <ha...@gmail.com>.
Hi,
It is important to have a contingency in case a reviewer requests changes
and then stops participating for whatever reason (for example, gets busy
with other work in the meantime). So, that's good thinking to include item
#2.
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.
Are you sure you'd like to go straight to a [VOTE] without a [DISCUSS]
thread first? Perhaps others may propose alternative contingency ideas, or
offer thoughts on why reviewers should be able to dismiss other reviewers'
change requests, so we can consider all aspects of this before we vote.
Cheers,
Nathan
On Tue, Nov 11, 2025 at 11:43 AM Matteo Golin <ma...@gmail.com>
wrote:
> 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
>
Re: [VOTE] Dismissing PR reviews - rules
Posted by Alin Jerpelea <je...@gmail.com>.
+1
On Tue, 11 Nov 2025, 17:42 Matteo Golin, <ma...@gmail.com> wrote:
> 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
>