GeezerVison is showing github.com — Require keeping an unreviewed PR open for a period of time · Issue #541 · python/core-workflow
Require keeping an unreviewed PR open for a period of time #541DescriptionIn 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.
Reacted by Alex Waygood, Kirill Podoprigora and Pradyun Gedam Activityhauntsaninja commented on Jun 10, 2024
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 erlend-aasland commented on Jun 10, 2024 [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. AlexWaygood commented on Jun 10, 2024 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 erlend-aasland commented on Jun 10, 2024
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 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
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. sobolevn commented on Jun 10, 2024 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.
Reacted by Erlend E. Aasland AA-Turner commented on Aug 12, 2025 erlend-aasland commented on Sep 18, 2025 I still think we should consider this.
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|