Maintainer-Guidelines: give maintainers time to review enhancements.

[MikeMcQuaid]

Let's slow down a little on enhancement PRs to give other maintainers time to give their feedback.

This is mostly self-directed criticism after discussion with @sjackman and some other @homebrew/plc members.

@Homebrew/maintainers please provide your thoughts and feedback here. I only want to merge this if we have majority agreement on it.

• Have you followed the guidelines in our Contributing document?
• Have you checked to ensure there aren't other open Pull Requests for the same change?
• Have you added an explanation of what your changes do and why you'd like us to include them?
• Have you written new tests for your changes? Here's an example.
• Have you successfully run brew style with your changes locally?
• Have you successfully run brew tests with your changes locally?
• Have you successfully run brew man locally and committed any changes?

---

[iMichka]

Agreed that we need more time to review things: the pace is quite high, and stuff is moving fast.
Another way to handle this is to say that in general you would need an approval instead of having a time limit. 24 hours might also be too short in my opinion.

[dawidd6]

I would also add to the list any "refactoring" PRs. I think they need to wait some time until they are reviewed by some more people, cause they sometimes break things and create needless frustration.

[MikeMcQuaid]

Agreed that we need more time to review things: the pace is quite high, and stuff is moving fast.
Another way to handle this is to say that in general you would need an approval instead of having a time limit. 24 hours might also be too short in my opinion.

We tried approvals in the past and I don't think they are a great fit for several reasons:

• I don't think we need (yet, at least) a process where you need a random maintainer who may have no context to ✅ your PRs to merge if CI is green
• 24h in the work week seems like a happy middle ground between "providing time for people to raise any objections" and "not slowing down folks like @MikeMcQuaid and @reitermarkus who do large refactorings and wish to self-merge their PRs without approval"

If it helps: the 24h is broadly based off the principle in earlier GitHub days to allow a geographically distributed team to be able to do the above. We now have more required ✅ for compliance reasons

I would also add to the list any "refactoring" PRs. I think they need to wait some time until they are reviewed by some more people, cause they sometimes break things and create needless frustration.

@dawidd6 I would consider "refactoring" to be an "enhancement" here but I'll add them specifically 👍🏻

[SMillerDev]

We could make it approval or 24h, whichever comes first.

[MikeMcQuaid]

We could make it approval or 24h, whichever comes first.

@SMillerDev I would specifically like to have maintainers be able to provide feedback on PRs that may be approved by other maintainers before they are merged. In my case I want (and try) to read all Homebrew/brew PRs so I would like PRs submitted in my evenings/weekends to give me 24h to be able to take a look. It sounds like there's some desire on the Homebrew/core side for something similar.

[maxim-belkin]

I'm +100 to giving other maintainers time to review. I wouldn't limit this to 24 hours though but instead to one, two or three approvals (depending on the repo) from other maintainer(s) or to three business days. In this case, something like "three business days or N approvals" seems like a reasonable requirement.

I second what @iMichka says here:

Another way to handle this is to say that in general you would need an approval instead of having a time limit. 24 hours might also be too short in my opinion.

and what @dawidd6 says here:

I would also add to the list any "refactoring" PRs. I think they need to wait some time until they are reviewed by some more people, cause they sometimes break things and create needless frustration.

I don't think the following justification applies to Homebrew, @MikeMcQuaid:

the 24h is broadly based off the principle in earlier GitHub days to allow a geographically distributed team to be able to do the above.

because GitHub employees are paid to do that (this is their job) and we are not.

With regard to:

• I don't think we need (yet, at least) a process where you need a random maintainer who may have no context to ✅ your PRs to merge if CI is green

This can be circumvented by providing instructions on how to test and by explaining the expected change in the behavior so that the PR can be tested even by a maintainer without a context. The goal of doing that is to make sure that the changes don't affect the work of that other maintainer.

Regarding:

• 24h in the work week seems like a happy middle ground between "providing time for people to raise any objections" and "not slowing down folks like @MikeMcQuaid and @reitermarkus who do large refactorings and wish to self-merge their PRs without approval"

These folks could help each other by reviewing each other's work... In that case they wouldn't have to wait 24 hours (if N _{required approvals} = 1).

Finally, we need to set what an "approval" should look like. In case of Homebrew (because so many people use it on a daily basis), I think the reviewer should explain how exactly they tested the PR. Empty approvals should not be counted towards the approvals count.

[claui]

I think the reviewer should explain how exactly they tested the PR. Empty approvals should not be counted towards the approvals count.

I like the idea of logging the review steps I take, and writing better approval comments.
Let’s remember that habit-forming is super hard though, and takes time and patience.
Therefore we should be picky when adding new hard requirements IMHO.

[fxcoudert]

I'm a bit worried that adding more and more explicit rules (so many days, so many approvals) is creating a bureaucracy in which nothing can ever advance, or require extraordinary effort.

I'm not a very frequent contributor to homebrew/brew, but on the homebrew-core side things we are definitely lacking maintainer time: it's common that PRs become stale halfway through a discussion, because nobody had time to help, could review, could suggest a way forward, etc.

Edit: I would add that there is no way in our workflow to say “I approve but don't merge until 24 hours / 1 work day have elapsed”, so that rule would be creating additional mental load for maintainers: you have to come back to this PR one day later to actually approve it (because if you approve it now it will ship if CI is green). That's not optimal.

Edit 2 (sorry): I feel for this brew is very different from homebrew-core. brew is our common architecture, and I totally agree with the proposed changes when it comes to brew. For homebrew-core, all formulas are pretty independent and (if CI passes tests) then any further improvement can always be done later (and have a generally much lower impact).

[MikeMcQuaid]

I wouldn't limit this to 24 hours though but instead to one, two or three approvals (depending on the repo) from other maintainer(s) or to three business days. In this case, something like "three business days or N approvals" seems like a reasonable requirement.

@maxim-belkin Three business days is much harder to keep track of than one. Again, I would like to avoid "N approvals" because you can get any number of approvals but still be missing maintainers who have important context.

because GitHub employees are paid to do that (this is their job) and we are not.

My argument is allowing people a reasonable amount of time to provide feedback if they wish to be sufficiently engaged on a given issue/PR/repo. I consider one working day to be that

I'm a bit worried that adding more and more explicit rules (so many days, so many approvals) is creating a bureaucracy in which nothing can ever advance, or require extraordinary effort.

@fxcoudert I agree.

Accordingly: I won't be adjusting this proposal to be approval-based because I don't agree that approach is necessary unless reviews are individual solicited. For enhancements/refactorings I just want to provide some time for people to review that's longer than how long it takes CI to run and accounts for the fact we have a wide timezone distribution between maintainers.

My take is in homebrew-core most PRs look more like "version bumps" or "fixes" than enhancements. Perhaps that's not the case? If so, I'm happy to see this adjusted accordingly.

[SMillerDev]

I don't think this is needed for core, we have so little actual enhancements that most are reviewed extensively before merging.

---

While I don't share your fear of irrelevant reviews because I think the motivation is different, I think a timer is fine too. I would just like to see a github action that helps us there in that case.

[scpeters]

I think a timer is fine too. I would just like to see a github action that helps us there in that case.

an automated timer would be clever, and it begs the question of which timezone is used in the "5pm on Friday" / "5pm on Monday" examples

[MikeMcQuaid]

I don't think this is needed for core, we have so little actual enhancements that most are reviewed extensively before merging.

@SMillerDev Fair point. Do you think it's worth either:

1. excluding core
2. making clear from the description that core enhancements are so rare that they'll easily meet the criteria here

an automated timer would be clever, and it begs the question of which timezone is used in the "5pm on Friday" / "5pm on Monday" examples

My thinking with the "24 hours" is in the situation where I, as a maintainer, wish my PR to be merged as soon as possible with or without review. 24 hours gives me a minimum amount of time I leave the PR so I can solicit input from other maintainers who may wish to request changes on the PR before it is merged. I can merge it whenever I want but I should wait 24h to give those maintainers time (given timezones, sleeping patterns) to do so.

[iMichka]

Also: we do have releases in brew, so this delays the time where stuff is shipped to all our users, leaving time for a post-merge review.

So 24h is somewhat fine, unless we need to do an emergency release in-between (but I doubt this happens that often).

I would definitively remove core from this recommendation.

[maxim-belkin]

I think approvals are essential (especially for open-source projects) to make sure the changes are tested in scenarios different from those used by the submitter. Of course, if a submitter uses all features of Homebrew/brew, maintains their own tap and Homebrew/core and/or migrates/deprecates/deletes formulae (and their features) on a regular basis -- then sure, 24 hours is enough. Otherwise, a second pair of eyes is essential.

Also, somewhat tangentially related: I feel that at least sometimes Homebrew/brew needs to request approvals from Homebrew/core maintainers to make sure changes to Homebrew/brew don't disturb their workflows that are not captured by the test-bot. As an alternative, we could devise some requirements for PR to allow self-merging, e.g.: "To merge your own PR, please provide one|two|three scenarios in which you tested the changes so that other maintainer can later confirm your observations. Preferably, scenarios would make use of Homebrew/brew and/or Homebrew/homebrew-core".

Finally, regarding "merging as quickly as possible": I'm not sure if this is a good goal. Homebrew is mature (and popular) enough for people (users & developers) to expect that it'll just work / continue to work. So, while I understand that even 24-hour time limit is already some self-restriction, I think that it should be closer to 48 hours.

[MikeMcQuaid]

I think approvals are essential (especially for open-source projects) to make sure the changes are tested in scenarios different from those used by the submitter.

I think we have a different definition of "essential" considering this project has run for over 10 years for millions of users without most PRs having approvals 😉

"To merge your own PR, please provide one|two|three scenarios in which you tested the changes so that other maintainer can later confirm your observations. Preferably, scenarios would make use of Homebrew/brew and/or Homebrew/homebrew-core".

I think this goes without saying, honestly.

Finally, regarding "merging as quickly as possible": I'm not sure if this is a good goal. Homebrew is mature (and popular) enough for people (users & developers) to expect that it'll just work / continue to work. So, while I understand that even 24-hour time limit is already some self-restriction, I think that it should be closer to 48 hours.

24h is fine because:

Also: we do have releases in brew, so this delays the time where stuff is shipped to all our users, leaving time for a post-merge review.
So 24h is somewhat fine, unless we need to do an emergency release in-between (but I doubt this happens that often).

For reference, ~0.01% of users are on the master branch and affected by 24h merges. In my experience having regular merges encourages smaller pull requests and faster development. This is a desirable goal in itself; it just needs balanced against product and code quality.

I would definitively remove core from this recommendation.

@iMichka (and @SMillerDev again): Do you think it's worth either:

• excluding core
• making clear from the description that core enhancements are so rare that they'll easily meet the criteria here
• being explicit about the other types of core PRs that are fine to self-merge

I guess I'm still thinking on core if people decide to e.g. change the MySQL datadir, start using an optional dependency by default, etc. they should have 24h to be reviewed. If that's already happening: great, we can document that and it doesn't change any behaviour. If it's not: I think we should have the same policy as Homebrew/brew for the same reasons (arguably it's more important given a brew upgrade makes often non-idempotent changes to people's installations).

[SMillerDev]

I'd go for this option:

making clear from the description that core enhancements are so rare that they'll easily meet the criteria here

[MikeMcQuaid]

I'd go for this option:

@SMillerDev done in 57e8f34

[MikeMcQuaid]

Feels like we've reached something mostly acceptable here so let's merge and feel free to offer specific wording changes as a follow-up PR.