-
Notifications
You must be signed in to change notification settings - Fork 984
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
* Added PR Review Policy * Reformat of Policy section to headline with policy fundementals * Renaming code owner to requester
- Loading branch information
1 parent
c35541a
commit 04d2d9a
Showing
3 changed files
with
99 additions
and
0 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,96 @@ | ||
## Pull Request Review Policy | ||
|
||
This document details the pull request policy of the Status Mobile repository / team. | ||
|
||
- [Terms](#terms) | ||
- [Policy](#policy) | ||
- [Overview](#overview) | ||
- [Full Policy](#fully-policy) | ||
- [Rationale](#rationale) | ||
- [Front pressure and Demand control](#front-pressure-and-demand-control) | ||
- [Back pressure and Supply control](#back-pressure-and-supply-control) | ||
- [Honour System](#honour-system) | ||
- [Review Support](#review-support) | ||
- [Background](#background) | ||
|
||
### Terms: | ||
|
||
| Term | Definition | | ||
|----------------------|---------------------------------------------------------------------------------------------------------------| | ||
| **Requester** | The team member submitting a pull request. | | ||
| **Reviewer** | A team member asked by a requester to review the requester’s pull request. | | ||
| **Review bandwidth** | The number of reviews a team member has self-limited themselves to giving within their defined review period. | | ||
| **Review period** | The period of time over which a reviewer’s bandwidth is consumed | | ||
|
||
### Policy: | ||
|
||
#### Overview | ||
|
||
- 🔀 - You are responsible for getting your PR reviewed and approved. | ||
- ✅ - You must give a review on a PR if you are asked to do so, unless your review limit has been reached. | ||
- ✨ - The honour system underpins this policy, without acting in good faith it will not work. | ||
|
||
#### Fully policy | ||
|
||
1) The requester has the responsibility for getting a PR reviewed and approved. | ||
1) The requester has the right and responsibility to pursue code reviews. | ||
2) The requester has the right and responsibility to make multiple requests of the same reviewer. | ||
3) The requester will select a limited number of reviewers to review any of the requester’s PRs. | ||
2) Reviewers are obligated to give a review for all PRs they are requested to review, except when their review bandwidth has been exhausted: | ||
1) Reviewers have the right to refuse a requester a review if the reviewer’s review bandwidth has been exhausted. | ||
2) Reviewers are expected to manage and track their review bandwidth. | ||
3) Reviewers can manage their review bandwidth via any means they feel works best for them. Examples include: | ||
1) Using the GitHub busy feature | ||
2) Declining a review on the PR | ||
3) Declining a review when pursued by a code reviewer | ||
4) Proactively declining to review a PR | ||
3) All team members will set a review bandwidth that is reasonable. | ||
4) All team members acknowledge that giving reviews is a requirement of membership of the team. | ||
5) All team members will manage their review bandwidth honestly. | ||
1) The right to refuse a PR review is only eligible if a reviewer’s review bandwidth has actually been exhausted. | ||
2) Breaching the team’s honour is very bad form. Do not do it (please). | ||
6) All team members acknowledge that without acting with honesty this review process will not work and jeopardises their own future ability to receive reviews. | ||
|
||
### Rationale | ||
|
||
#### Front pressure and Demand control | ||
|
||
Foremost in the rationale for the above policy is that giving a PR review is a requirement. If a requester requests | ||
a PR review of a team member, the reviewer is now obligated to review the PR within a reasonable timeframe. | ||
|
||
This principle enforces that there is a pressure and demand that PR reviews must be given. | ||
|
||
#### Back pressure and Supply control | ||
|
||
Front pressure alone is not enough to balance the PR review load across the team. Because of this an exception is granted | ||
to reviewers that have exhausted their review bandwidth, the exemption permits these reviewers to refuse a PR review. | ||
|
||
This principle gives the reviewer pool back-pressure, relieving the exhausted reviewer and increasing pressure / demand on | ||
the other reviewers. | ||
|
||
#### Honour system | ||
|
||
The two principles of demand and supply control are underpinned by an honour system and are useless without honest action. | ||
If a team member does not honestly manage their review bandwidth this action seriously damages the collective balance of | ||
PR review load. Because of this potential damage a breach of the honour system is considered a deeply shameful act. | ||
|
||
The honour system is the mechanism of control and enforcement, without it the policy would need to be controlled and enforced | ||
using less dynamic and forgiving mechanisms. An example is a GitHub review counting tools with quotas and metrics. So just | ||
be cool. | ||
|
||
### Review Support | ||
|
||
Sometimes reviewing is time-consuming or generally painful, to help make reviews as simple as possible consider the following suggestions: | ||
- Paired reviews: Ask an experienced reviewer to review with you. | ||
- If a PR is the product of multiple developers only ask 1 or 2 other developers for their review. | ||
|
||
### Background | ||
|
||
The contents of this policy document is the product of 2 councils of the Status Mobile team, the below documents summarise the topics and outcomes of the councils: | ||
|
||
- [2023-05-15 - Council of Deadwater](https://docs.google.com/document/d/1TtUZCke2OzumjdWGu6TIECHEOUPwvaLrvQ8Re2nEGCo) | ||
- [2023-06-13 - Mauve Council of Deadwater](https://docs.google.com/document/d/19hfr8xXrShLk9ROdQfOwb9_vwbTah9nOCqKSLIrgoSI) | ||
|
||
The aims of the council were to allow developers that are relied on heavily for reviews to better balance their time | ||
between reviews and other responsibilities. Equally, the council aimed to give other team members better opportunity and | ||
understanding that giving PR reviews is a requirement and will help the team generally if more review burden is shared. |