Dismiss review if a PR is modified
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 :-)
Victor
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).
Oh.
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.
Victor
Le 19 juil. 2017 1:13 AM, "Brett Cannon" <brett@python.org> a écrit :
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).
On 19 July 2017 at 09:37, Victor Stinner <victor.stinner@gmail.com> wrote:
Oh.
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 ;-)
Right, "review required" settings can be useful, but they genuinely require a self-review option as an escape hatch in community projects, and GitHub doesn't currently offer that (it doesn't allow self-review at all, not even to mark your own PRs as still requiring further changes).
Cheers, Nick.
-- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia
On Tue, 18 Jul 2017 23:13:41 -0000, Brett Cannon <brett@python.org> wrote:
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).
Ah, I thought we were already doing that, but of course we aren't. Nevermind :)
--David
On Wed, 19 Jul 2017 at 08:18 R. David Murray <rdmurray@bitdance.com> wrote:
On Tue, 18 Jul 2017 23:13:41 -0000, Brett Cannon <brett@python.org> wrote:
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 ).
Ah, I thought we were already doing that, but of course we aren't. Nevermind :)
I was planning on bringing this up in a year or so once everyone was comfortable with the workflow. :)
participants (4)
-
Brett Cannon -
Nick Coghlan -
R. David Murray -
Victor Stinner