Maintainer Partial Review Guidelines

[Princess-Cheeseballs]

Overview

Adds a partial review guidelines section to Review Policy.

These guidelines are meant to improve communication between maintainers when a maintainer requests help on a PR that may be out of their area of expertise, as well as when a maintainer has comments on a PR they don't plan to fully review.

It's also meant to hopefully help speed up the review process by letting maintainers not have to dedicate themselves to a full review for PRs within their area of expertise, and communicate that to other maintainers so they can pick up these PRs later.

Why

There's a lot of reasons why genuinely.

This is already kind of a soft policy but it doesn't work very well since it's not organized and extremely taxing. We have PRs we call "Sloth PRs" PRs only Slarti can review, PRs technical enough we need a wizard or lead maintainer to review them and right now the system of pinging and review begging doesn't work.

Stuff slips through the cracks, things get left behind, work piles up and eventually things get merged without proper review because that's the only option and all you can do is hope that it works based on the research, review, and testing you've done.

The guidelines change aim to optimize and streamline a big undocumented part of the review process to hopefully make it more common and also speed up reviews on larger PRs.

This policy aims to do this by:

• Defining when a maintainer should think about asking for a partial review.

• Defining how a maintainer who is choosing to do a partial review over a full review should go about doing it such that other maintainers are aware of their intentions.

• Defining partial reviews so maintainers can leave reviews without having to worry about being expected to re-review later. And so stale reviews can be more easily dismissed once they're addressed.

I do think we could also use some more teach and learn policies. But this should help a decent amount. If there's any questions about the guidelines leave them as a review so I can rewrite them to hopefully answer any questions.

[VerinSenpai]

should there be any clarification about what has been reviewed vs what hasn't?

[Princess-Cheeseballs]

should there be any clarification about what has been reviewed vs what hasn't?

As in if a deferral was part of a review? Probably yeah.

[K-Dynamic]

Should probably specify when and why a deferral exists.

e.g. the code/balance/licensing is too complicated to explain or understand but someone else knows

[K-Dynamic]

1. If there's a lot of deferrals to the same people for the same code/balance/licensing issue, that could indicate a lack of knowledge among staff; it might be worth trying to train or teach staff said missing knowledge so there are fewer deferrals.
2. Would deferrals share equal responsibility for a review or are they only responsible for the sections that have reviewed?

[slarticodefast]

The text reads good, but I would consider moving it into a separate doc with best practices or similar and not calling it a policy. In the past we had maintainers that were uncomfortable with accidentally making mistakes and breaking the policy, and each time we make the policy longer it gets more complicated and bureaucratic. So we should really emphasize that this is meant as a "best practices" recommendation and not as a "you must follow this policy our you will be de-maintainered".

[juliangiebel]

I think this is fine as an extension to the policy because people shouldn't have to read some "best practices" document in addition to the policy to know that it's fine to not fully review PRs. I also don't agree with the current policy and this addition being a problem. If anything these additions actually make the policy less restrictive.

I do agree that we need to be really careful about the amount of policies and how difficult they are to comprehend.

In the past we had maintainers that were uncomfortable with accidentally making mistakes and breaking the policy

Nobody gets told off for accidentally breaking the policy. Mistakes happen. Past incidents with complaints against maintainers all haven't been because the policy wasn't followed.

[Princess-Cheeseballs]

The text reads good, but I would consider moving it into a separate doc with best practices or similar and not calling it a policy. In the past we had maintainers that were uncomfortable with accidentally making mistakes and breaking the policy, and each time we make the policy longer it gets more complicated and bureaucratic. So we should really emphasize that this is meant as a "best practices" recommendation and not as a "you must follow this policy our you will be de-maintainered".

I put it in the guidelines section which is not policy but instead closer to a "best practices" section which I feel is better.