Don't close PRs waiting for reviews

[jablko]

If the author has re-requested a review, don't close the PR as abandoned while awaiting a response.

[elibarzilay]

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?

[jablko]

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

1. The other criteria still apply (maintainer/owner/other approval)
2. The alternative potentiality is someone issuing a changereq and then not responding until the bot closes the PR
3. A changereq is only good until the next push anyway

[elibarzilay]

I spent more time thinking about this, and I still don't see the point.
But the more I look the more I see arguments against it.

1. It has a fundamental complication of an implementation that just
   ignore the review, and therefore results in more code that will need
   to change. For example, having this PR end in an Unreviewed state
   is bogus, since it was reviewed, and the reviewer asked for
   changes.

2. Looks like this was a result of @amcasey slightly misusing the system
   by submitting a changereq but ended up being happy with just
   clarifications and no actual changes. Even if this wasn't the case,
   since it's such an edge case, you could have avoided the closure by
   just pushing an update, which would achieve a similar result. (It's
   a hack, but it's a rare case to begin with.) Such an update would
   make the bot ask for an updated review.

3. Ignoring the contents, this PR actually allows potentially bad
   behavior: A does a changereq for your PR and B approves it, you
   dismiss the changereq by asking for a re-review, and now you can
   merge it since now it has just the approval. The bad thing here is
   that you could ignore changereqs this way.

   Again, it is possible to do this anyway, by pushing a dummy update,
   nut since you're already updating the PR, you'd usually try to adjust
   code according to the request anyway. Also, such an update would
   delay closing the PR in a very similar (except that then an
   "Unreviewed" label would actually make sense).

4. Finally, without this, and even without a dummy update to game the
   system, you'd normally ping the reviewer and note that you replied to
   the questions and no change was needed, and that would reset the
   activity clock, and encourage the reviewer to have a second look and
   either stick to the request or change it to passing.

Given all of this, I now think that this change should not get merged.

[elibarzilay]

(Pushed a rebased + squashed version since I did it anyway.)

[jablko]

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 Waiting for Code Reviews/Needs Maintainer Review (where a response is more likely). If the reviewer doesn't reply back it'll get an Abandoned label and eventually get closed.

With this change it does move columns. If the reviewer doesn't reply it gets an Unreviewed label and eventually ends up in Needs Maintainer Action, which I still think is an improvement?

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:

• I'm not sure contributors know to do that in this situation
• I'm not sure the bot should assume a push resolves a changereq (and a reply does not)

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), Unreviewed is less bogus than Abandoned? I'm not sure any other code changes are needed?

[elibarzilay]

Just some quick replies:

1. Normally people would ping the reviewer in a comment anyway. If it gets closed, there's also the simple option of re-opening it.

2. It shouldn't be robust against malicious behavior --- I'm more concerned about unintentionally bad behavior: I request a review from A, B authorize it, I'm so happy I quickly merge it, A comes back and complains about ignoring that review.

[jablko]

Do you have to be a maintainer to reopen a PR once the bot closes it?