[Matplotlib-devel] clearing 'request changes' review

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

[Matplotlib-devel] clearing 'request changes' review

tcaswell
Folks,

I propose the following:

 - if you would be happy with any other dev clearing the review leave a note saying so explicitly
 - the default (no note) is to not dismiss other devs reviews, but if there is otherwise consensus and the 'request changes' reviewer is non-responsive, it may be cleared (use your judgement folks!)
 - if you want to block merging until you re-review it, leave a note saying so.

Tom

_______________________________________________
Matplotlib-devel mailing list
[hidden email]
https://mail.python.org/mailman/listinfo/matplotlib-devel
Reply | Threaded
Open this post in threaded view
|

Re: clearing 'request changes' review

Nelle Varoquaux


On 11 February 2017 at 14:17, Thomas Caswell <[hidden email]> wrote:
Folks,

I propose the following:

 - if you would be happy with any other dev clearing the review leave a note saying so explicitly
 - the default (no note) is to not dismiss other devs reviews, but if there is otherwise consensus and the 'request changes' reviewer is non-responsive, it may be cleared (use your judgement folks!)
 - if you want to block merging until you re-review it, leave a note saying so.

I have been recently chasing people to re-do reviews as some requests changes but don't follow up on their reviews. Can reviewers either be more responsible in checking updates on PR's that they block, or do "soft request changes" by commenting and not requesting changes? Some PRs have been in stalled status for several weeks as now, instead of just having to chase the contributor, we now also have to chase the reviewers.

Thanks,
Nelle


Tom

_______________________________________________
Matplotlib-devel mailing list
[hidden email]
https://mail.python.org/mailman/listinfo/matplotlib-devel



_______________________________________________
Matplotlib-devel mailing list
[hidden email]
https://mail.python.org/mailman/listinfo/matplotlib-devel
Reply | Threaded
Open this post in threaded view
|

Re: clearing 'request changes' review

Benjamin Root
I think part of the problem is that github doesn't always send a notification that the contributor has made changes since the last request for changes. Sometimes it does, and sometimes it doesn't. I haven't figured out the difference for sure yet. My theory is that notifications aren't sent when a reviewer squashes their commits, but I am not certain.

Ben Root

On Fri, Mar 24, 2017 at 12:44 PM, Nelle Varoquaux <[hidden email]> wrote:


On 11 February 2017 at 14:17, Thomas Caswell <[hidden email]> wrote:
Folks,

I propose the following:

 - if you would be happy with any other dev clearing the review leave a note saying so explicitly
 - the default (no note) is to not dismiss other devs reviews, but if there is otherwise consensus and the 'request changes' reviewer is non-responsive, it may be cleared (use your judgement folks!)
 - if you want to block merging until you re-review it, leave a note saying so.

I have been recently chasing people to re-do reviews as some requests changes but don't follow up on their reviews. Can reviewers either be more responsible in checking updates on PR's that they block, or do "soft request changes" by commenting and not requesting changes? Some PRs have been in stalled status for several weeks as now, instead of just having to chase the contributor, we now also have to chase the reviewers.

Thanks,
Nelle


Tom

_______________________________________________
Matplotlib-devel mailing list
[hidden email]
https://mail.python.org/mailman/listinfo/matplotlib-devel



_______________________________________________
Matplotlib-devel mailing list
[hidden email]
https://mail.python.org/mailman/listinfo/matplotlib-devel



_______________________________________________
Matplotlib-devel mailing list
[hidden email]
https://mail.python.org/mailman/listinfo/matplotlib-devel