GeezerVison is showing github.com — Review dismissal · Issue #36 · python/core-workflow
DescriptionIt appears that a 'request changes' review blocks merge even if another core-dev approves. I'm not sure whether two approvals would override a 'request changes', but it is possible to dismiss a review; we should decide criteria for dismissing another core-dev's review here and document it in the devguide.
For example, python/cpython#27 was blocked by my outdated review, even though @methane had made the changes I had requested and @ncoghlan had approved the change. Frankly, I had forgotten that I had reviewed that PR, so I would have had no problem with either of the other involved core-devs dismissing my review. Activitymethane commented on Feb 20, 2017 I tried "dismiss review" link. python/cpython#75
When I clicked the link, one line textbox for writing reason to dismiss.
I wrote "fixed" there. facundobatista commented on Feb 20, 2017 El 19/02/17 a las 20:54, Zachary Ware escribió:
It appears that a 'request changes' review blocks merge even if another core-dev approves. I'm not sure whether two
approvals would override a 'request changes', but it is possible to dismiss a review; we should decide criteria for
dismissing another core-dev's review here and document it in the devguide.
IMO, no matter how many approves a PR gets. If one core-dev request changes, the merge should be blocked.
This is a normal situation, where the PR looks ok for everybody but for somebody (probably the feature owner) that finds
an important detail. The PR shouldn't merge until that is resolved.
The only risk here is that the person that requested the change stops paying attention and the PR stalls even after the
fixes are there. This is an extreme situation, and if the original core-dev that requested the change never answers
back, the situation can be overruled (for example, by proposing another PR and just accepting it); it's more a social
than a technical problem.
Regards,
…ncoghlan commented on Feb 23, 2017 I think in our case, we need to assume that core developers losing track of a change request is going to pretty normal, and establish that in those cases it's socially acceptable for another dev to say "I've checked the requested changes were made" and dismiss the original review. (It's unfortunate that GitHub chose such a negative phrasing for their UI, but it makes sense in a less collaborative corporate context where reviewers don't issue an assumed delegation to their peers).
I'd say the only time we shouldn't dismiss a review after the requested changes have been made is when the reviewer also assigns the PR to themselves - that's a more active "I'm handling this" claim than just requesting some changes. facundobatista commented on Feb 26, 2017 brettcannon commented on Feb 27, 2017 So do we want to write this down in the devguide? Is there anything specific that needs to happen to keep this issue open for? dstufft commented on Feb 28, 2017 Documenting it in the dev guide seems like a reasonable thing to do. zware commented on Feb 28, 2017 If we're in agreement on what our convention should be, this issue should be replaced with an issue for documenting it in the devguide. Reacted by Brett Cannon and Mariatta brettcannon commented on Apr 10, 2017
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|