Repository navigation
Review dismissal #36
Description
Activity
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.- 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,…-- . Facundo . Blog: http://www.taniquetil.com.ar/plog/ PyAr: http://www.python.org.ar/ Twitter: @facundobatista
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.
Reacted by Xiang Zhang+1
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?
Documenting it in the dev guide seems like a reasonable thing to do.
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 MariattaOpened python/devguide#161.
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.
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.