Proposal: Changing the review policy

[eladyn]

Currently, the master-branch is set up to require changes through pull requests and any PRs to master must have at least to approving reviews from maintainers before they can be merged. The reason for this can be found here.

Due to various reasons that I'll explain below, I'd propose relaxing that policy to require only one approving review before merging is allowed.

• To use myself as an example: Currently, I've got quite some time to contribute to spotifyd and review pull requests. However, this hadn't been that way the past few months. I still tried to somehow keep up with the project's activity, but it could well take a week until I got around to taking a proper look at a PR.
  While this is totally fine, considering this is a community-driven open source project, it can lead to contributors being discouraged from spending their time with improvements of the project.¹
• The original reason – I think –, why the policy has been set up the way like that, is that it was unclear how well it would go with the new maintainers. Kind of a “security measure”. I would argue that it stood the test of time and worked out in a good way, so we might no longer need that level of caution.
• Somehow tooting my own horn here²: PRs that are created by maintainers still require two approving reviews which is of course consequential, but considering that they are created by people that have some level of familiarity with the project, this can feel a bit superfluous.

Still, I think having a good amount of reviews is always beneficial, so my proposal would be the following: Reduce the technical restrictions to require only one reviewer for a PR to be merged. For PRs that aren't critical to be merged as fast as possible and aren't trivial³, we could leave a day or two of time between the last change and merging so that maintainers get enough time to weigh in, if they want to do that. And if maintainers would like to review the PR, but don't have time in the next few days, they could for example request a review from themselves to signal that to others.

In conclusion, we could keep the good style of having many people reviewing and approving, while avoiding stalling the project too much on technical restrictions.

If you got here, thanks for reading this long text! I'm happy to read other opinions on this.

Pinging the relevant people: @Spotifyd/maintainers, @Spotifyd/owners.

Footnotes

1. An example of that is this PR, which first didn't have enough reviewers to be merged and now has conflicts, which would need to be resolved. And we – of course – cannot expect contributors to return after such a long time and fix the conflicts, while it is unclear if the resolved version would then be merged or stall again. ↩

2. There are several open PRs by me that have one approving review and are missing a second one, so currently that only applies to myself. 😅 (remove workaround for rspotify id types #1145, update badges and add link to matrix #1172) ↩

3. Like spelling mistakes or link corrections, minor additions to the docs, ... ↩

[slondr]

I fully agree in spirit. I think adopting a convention of leaving time after approval and/or roping in additional reviewers for very consequential changes is sufficient for ensuring PRs which shouldn't be rushed aren't, but that doesn't really apply to most of them, lol.

At one point I looked into the possibility of people with merge permissions being able to approve their own PRs, which is a feature GitLab has. GitHub doesn't support that though, so I agree that generally relaxing the requirement to 1 approval is a good way to move forward.

Requiring multiple approvals for all PRs, big and small, might make sense if the maintenance team was much larger than it is, but this is still a hobby for most of us I think.

[eladyn]

I think adopting a convention of leaving time after approval and/or roping in additional reviewers for very consequential changes is sufficient for ensuring PRs which shouldn't be rushed aren't, but that doesn't really apply to most of them, lol.

Yeah. I think, waiting a day or something doesn't hurt in most cases, but we can also experiment with that and if someone feels that sometimes the merge button is pushed too fast, we can always discuss new conventions.

[SimonTeixidor]

I agree. I think we are not enough active maintainers to enforce two approvals per PR, so moving to one approval makes sense at this point. It must be pretty demotivating for new contributors to find that their PRs end up stuck due to this rule.

[eladyn]

Thank you both for your quick replies. It seems that reducing the reviewer count to 1 is rather uncontroversial, maybe we can change that tomorrow. 🚀 (Other people are of course still invited to comment on this matter, especially on merging conventions!)

[eladyn]

For the record: I just changed the setting. :)