[python-committers] Dismiss review if a PR is modified

Victor Stinner victor.stinner at gmail.com
Tue Jul 18 19:37:16 EDT 2017


For backports, it's convenient to be able to merge without a review. I see
many cores doing it and I like it.

For master, I don't know. Sometimes a PR is merged too fast, sometiles
nobody reviews a PR even if it's good. So for the master branch, the dev
takes its own responsability to merge ;-)

I only asked if it would be possible to dismiss an approval or mark the
review as outdated if the change is modified afterwards, but it seems like
GitHub doesn't offer many choices...

The current behaviour is okish. It was just a minor whish.


Le 19 juil. 2017 1:13 AM, "Brett Cannon" <brett at python.org> a écrit :

On Tue, 18 Jul 2017 at 13:10 Victor Stinner <victor.stinner at gmail.com>

> 2017-07-18 21:21 GMT+02:00 R. David Murray <rdmurray at bitdance.com>:
> > On Tue, 18 Jul 2017 12:24:13 +0200, Victor Stinner <
> victor.stinner at gmail.com> wrote:
> >> I'm just not unconfortable with the fact that an approval is kept even
> >> if the PR is modified after the review :-/ I would expect a list a
> >> notice "changed modified after the review" or something like that. At
> >> least, for my own reviews, to remind me to review again.
> >
> > This could be changed if we have consensus to do so.  Github has a
> > setting that will cause existing approvals to be "dismissed" if
> > a new commit is pushed.
> >
> >> Compared to Rietveld, GitHub review tool is as "a good" (not much
> >> better, not much worse). Sometimes, I'm lost in reviews: my comments
> >> are hidden, I have to unfold many widgets. But it's not that bad. It
> >> seems like avoiding rebase works around some of these issues.
> >
> > I much prefer rietveld over github reviews, but I also much prefer the
> > integration between the bug tracker and github over the minimal
> > integration we had for rietveld.  Thanks to all the people who made
> > that happen, and especially Brett for leading it.
> I am in favor of making this change :-)

Do realize that setting is part of requiring a review for pull requests:
reviews-for-pull-requests/. So in order to get this we would require all
PRs, core dev or not, to receive an approving review from another core
developer (I don't think an approving review from just anyone counts
towards the minimum approval). There might be around this, but it will
require some testing to make sure (see https://github.com/python/
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.python.org/pipermail/python-committers/attachments/20170719/b117da76/attachment-0001.html>

More information about the python-committers mailing list