Closed elibarzilay closed 3 years ago
I've talked about this with @RyanCavanaugh. And we agreed that there might be a problem with maintainers being surprised when the bot immediately merges once an approval is in if they missed an early request to merge.
However, on the flip side there is the problem of making it into a nagging argument-with-a-machine kind of a thing, similar to an unwanted "are you sure you want to quit?" questions. This is also because having further discussions and passing reviews shouldn't in general make any difference for an author's decision that they want to merge the PR.
So it seems like it's best to keep things as they are now. The implication for the dt maintainers is to generally be aware that if there is a valid (= post-first-approval) request for merge before a PR could be merged, then an approving review or a blessing could lead to an immediate merge.
For the record, there are two variants of the above suggestions:
Similar outline, but instead of requiring a request after the last review, require it to be the last interaction, so post any other comments etc. On one hand, this minimizes any surprises in that the bot would only merge PRs with a request at the bottom, and on the other hand, this would increase human-bot chatter of the above unpleasant kind.
Another variation of the above is to keep things as they are but add the reminder to re-ask for a merge once an old one is revoked (when it is mergeable, and an old request exists before the first review). This doesn't seem to be worth it though since most people would probably expect this so no need for the increased chatter.
The problem is that using
firstApprovalDate
can kick in when the first approval is irrelevant for Merge:Auto, so what can happen, as demonstrated in DefinitelyTyped/DefinitelyTyped#48652 is:To solve this:
lastApprovalDate
instead, so the above scenario would ignore the request.sinceDate
argument (which uses the above), post a JFYI for A,B,C (with comment that the others can do so too) that the earlier request is ignored due to new activity, and it should be re-posted.sinceDate
, look for the requests that are before that date and after the last "JFYI" note. This would avoid repetitions.The above scenario should be "fixed" even if: