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

2017-07-18 21:21 GMT+02:00 R. David Murray <rdmurray@bitdance.com>:
> On Tue, 18 Jul 2017 12:24:13 +0200, Victor Stinner <victor.stinner@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: https://help.github.com/articles/enabling-required-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/core-workflow/issues/94#issuecomment-316224864).