-
Notifications
You must be signed in to change notification settings - Fork 44
Don't close PRs waiting for reviews #344
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear to me what this thing is doing. A test (or at least a pointer to a PR that this will change) would be good.
Specifically, it's not clear to me what happens when there's a pending review request: can the PR continue to be held open infinitely? Or maybe if I have some strong objections in a request and you ask for a re-review -- is that effectively ignoring my changereq, which would allow you to proceed?
I added DefinitelyTyped/DefinitelyTyped#49667 (comment) as a test. The usual timelines apply, so after re-requesting a review a PR will eventually become "Unreviewed" and end up in "Needs Maintainer Action" (it won't be held open infinitely). You can currently hold a PR open indefinitely by repeatedly restarting the timeline (DefinitelyTyped/DefinitelyTyped#49667 (comment)), but you can't get it back into "Waiting for Code Reviews". Re-requesting a review would effectively dismiss a changereq, but
|
17adcf9
to
23ec8cc
Compare
If the author has re-requested a review, don't close the PR as abandoned while awaiting a response. * Update schema * Add test (49667) Co-authored-by: Eli Barzilay <[email protected]>
I spent more time thinking about this, and I still don't see the point.
Given all of this, I now think that this change should not get merged. |
(Pushed a rebased + squashed version since I did it anyway.) |
Thanks for thinking carefully about this. Yes (to point 4), if you reply to a changereq without pushing any changes you'd normally ping the reviewer and that would reset the activity clock, however it doesn't move it to With this change it does move columns. If the reviewer doesn't reply it gets an Yes also (to point 3), this change does allow bad behavior, although the bot already allows some bad behavior. I'm not sure it's the goal to be ironclad, as long as bad behavior is transparently antisocial to a human? In this case if you had an approval and a changereq, re-requested a review from the changereqer, and then merged the PR without giving them a chance to respond, that would be transparent to that reviewer (they'd be notified of the review request). I'm not sure it's the bot's role to police this? If however the reviewer ignored the request then I think it's reasonable to proceed based on other approvals/checks. @amcasey's use of the system seems like a natural one, just one that isn't currently supported (because the bot would've closed the PR), and this PR resolves that. I think re-requesting a review is preferable to a dummy push because:
However I do take your point that this case is rare. I'd still say if you re-request a review (after a reviewer asked for changes), |
Just some quick replies:
|
Do you have to be a maintainer to reopen a PR once the bot closes it? |
1d39a68
to
9e931d1
Compare
If the author has re-requested a review, don't close the PR as abandoned while awaiting a response.