Use SitOnIt-CI to enforce minimum lifetime for Pull Requests

[cmccandless]

I've noticed it's common in this repository to wait for a while before merging a PR that has been rapidly reviewed in order to give other maintainers a chance to comment themselves. At this time, this is more of an unwritten courtesy; what about making it policy?

SitOnIt-CI enforces just this, and is configurable per-repository.

I would suggest 24 hours as a required wait time; this is the default and requires no configuration.

As with any CI status service, any user with admin privileges on the repository may install the app or override it to merge anyway.

I would love to see some discussion about the pros/cons of enforcing wait times.

[rpottsoh]

I think this is an interesting idea. I am playing with it now on a test repo. So far I have been unsuccessful in getting it to work. As soon as the PR is created SitOnIt claims that the PR is old enough to merge.

Correction, I had a typo in my .sitonit.yml file. It works like a charm now. I had minute instead of minutes. With that spelling error PRs were considered old enough immediately. I assume because hours and days are set to zero (0) in my .yml file.

[rpottsoh]

This might also be a worthwhile discussion for the exercism/Discussions repo as well.

[petertseng]

I can imagine separate discussions being had for the policy versus the implementation.

Policy: I have been inconsistent about how long I've waited in the past, ranging from 72 hours to not waiting at all. I've had to make judgments on whether I think another maintainer might want to look at the PR in question and make up rules as I go along. If I could be spared the brainpower required to make up judgments and rules, that would certainly be good.

While I think there will be definitely be instances where 24 hours is not enough to give a certain individual a chance to look at a PR. It's why I usually give 72. But does it matter? Given the contents of this repo, perhaps it doesn't.

Implementation: Given I once made a math error, I suppose help would help prevent that. How many times will we need to disregard it and merge something ASAP? Probably not many given the contents of this repo. And that would have been the primary reason against.

[wolf99]

I can't think of the name of the tool, but the thing that is already used to close issues if they become stale... can this be made to cover PR's as well?

(sorry, I've forgotten the name of the tool, if I remembered it I'd be able to check this myself >.< )

[rpottsoh]

@wolf99 Probot. Review issues exercism/discussions#128 exercism/discussions#131 exercism/discussions#141. I may have missed some other discussions on the matter, but those stood out.

[cmccandless]

@wolf99 Stalebot does close stale PRs (optionally) as well already, but it cannot merge them. While SitOnIt would not merge automatically either (nor should it; I don't like the idea of auto-merging), it would mitigate premature merging.

[Insti]

(Removed policy label as there is no decided policy yet.)

[Insti]

I'm against adding an enforced minimum lifetime.

I'd be more worried about an important fix having to be delayed by a day just because some robot says so.

I think we do a good job of allowing time for others to review before merging.

[rpottsoh]

I agree. We also have WIP at our disposal as well, to sort of block merging until the PR is ready.