python
Require keeping an unreviewed PR open for a period of time
In order to encourage reviews, and to prevent PRs from being created and merged without reviews within a short period of time (sometimes within one hour!), it has multiple times been suggested (on Discord, informally in discussions on PyCon) to add tooling that prevents an unreviewed reviewed PR to be merged unless a predetermined period of time has passed.
I think we should add a check that for unreviewed PRs will require a certain period of time to pass before the PR can be merged. This check will reset if new commits are added. If a review is performed, the check is dismissed. If a review if performed and new commits are added, the check is reset and a new review must be performed in order to dismiss the check again.
For discussion:
- Should this check be dismissed only when a core dev or triager has reviewed the PR, or should we allow external reviews?
- How long should the delay be? I suggest somewhere between 1 and 2 weeks. IMO, 2 days or even 3 days is too short.
How long should the delay be
Maybe you could pull statistics from recent PRs so we know what the impact would be?
the check is reset
I'm worried this will force roundtrips that aren't particularly valuable
[additional context added by me]
If a review if performed and new commits are added, the check is reset and a new review must be performed in order to dismiss the check again.
I'm worried this will force roundtrips that aren't particularly valuable
It might do; it may also be valuable in some situations. If it turns out to be more of a nuisance, we can easily disable that functionality.
I like this idea overall, but I think it would be good to exclude docs-only PRs from this check.
If a review if performed and new commits are added, the check is reset and a new review must be performed in order to dismiss the check again.
I don't like this much, as I think it would disincentivise people from merging in main to the PR branch just before merging the PR. That's usually a good idea, as it helps prevent merge races where a outdated CI run is green on the PR but the tests end up failing on main
I like this idea overall, but I think it would be good to exclude docs-only PRs from this check.
That makes sense.
I don't like this much, as I think it would disincentivise people from merging in
mainto the PR branch just before merging the PR. That's usually a good idea, as it helps prevent merge races where a outdated CI run is green on the PR but the tests end up failing onmain
For external PRs, this is not a problem; the person merging in main will certainly be a core dev, and thus be in the position to immediately also approve the PR. For PRs created by core devs, it would of course be more irritating. I guess you are right it will quickly become a pain point.
We can use a comment with /recheck-review-status command or similar to use together with
If a review if performed and new commits are added, the check is reset and a new review must be performed in order to dismiss the check again.
