Add information on how the Synapse team does reviews. (#13132)

This commit is contained in:
Patrick Cloke 2022-07-06 07:30:58 -04:00 committed by GitHub
parent a0f51b059c
commit dcc7873700
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 47 additions and 1 deletions

1
changelog.d/13132.doc Normal file
View File

@ -0,0 +1 @@
Document how the Synapse team does reviews.

View File

@ -81,6 +81,7 @@
# Development # Development
- [Contributing Guide](development/contributing_guide.md) - [Contributing Guide](development/contributing_guide.md)
- [Code Style](code_style.md) - [Code Style](code_style.md)
- [Reviewing Code](development/reviews.md)
- [Release Cycle](development/releases.md) - [Release Cycle](development/releases.md)
- [Git Usage](development/git.md) - [Git Usage](development/git.md)
- [Testing]() - [Testing]()

View File

@ -351,7 +351,7 @@ To prepare a Pull Request, please:
3. `git push` your commit to your fork of Synapse; 3. `git push` your commit to your fork of Synapse;
4. on GitHub, [create the Pull Request](https://docs.github.com/en/github/collaborating-with-issues-and-pull-requests/creating-a-pull-request); 4. on GitHub, [create the Pull Request](https://docs.github.com/en/github/collaborating-with-issues-and-pull-requests/creating-a-pull-request);
5. add a [changelog entry](#changelog) and push it to your Pull Request; 5. add a [changelog entry](#changelog) and push it to your Pull Request;
6. for most contributors, that's all - however, if you are a member of the organization `matrix-org`, on GitHub, please request a review from `matrix.org / Synapse Core`. 6. that's it for now, a non-draft pull request will automatically request review from the team;
7. if you need to update your PR, please avoid rebasing and just add new commits to your branch. 7. if you need to update your PR, please avoid rebasing and just add new commits to your branch.
@ -527,10 +527,13 @@ From this point, you should:
1. Look at the results of the CI pipeline. 1. Look at the results of the CI pipeline.
- If there is any error, fix the error. - If there is any error, fix the error.
2. If a developer has requested changes, make these changes and let us know if it is ready for a developer to review again. 2. If a developer has requested changes, make these changes and let us know if it is ready for a developer to review again.
- A pull request is a conversation, if you disagree with the suggestions, please respond and discuss it.
3. Create a new commit with the changes. 3. Create a new commit with the changes.
- Please do NOT overwrite the history. New commits make the reviewer's life easier. - Please do NOT overwrite the history. New commits make the reviewer's life easier.
- Push this commits to your Pull Request. - Push this commits to your Pull Request.
4. Back to 1. 4. Back to 1.
5. Once the pull request is ready for review again please re-request review from whichever developer did your initial
review (or leave a comment in the pull request that you believe all required changes have been done).
Once both the CI and the developers are happy, the patch will be merged into Synapse and released shortly! Once both the CI and the developers are happy, the patch will be merged into Synapse and released shortly!

View File

@ -0,0 +1,41 @@
Some notes on how we do reviews
===============================
The Synapse team works off a shared review queue -- any new pull requests for
Synapse (or related projects) has a review requested from the entire team. Team
members should process this queue using the following rules:
* Any high urgency pull requests (e.g. fixes for broken continuous integration
or fixes for release blockers);
* Follow-up reviews for pull requests which have previously received reviews;
* Any remaining pull requests.
For the latter two categories above, older pull requests should be prioritised.
It is explicit that there is no priority given to pull requests from the team
(vs from the community). If a pull request requires a quick turn around, please
explicitly communicate this via [#synapse-dev:matrix.org](https://matrix.to/#/#synapse-dev:matrix.org)
or as a comment on the pull request.
Once an initial review has been completed and the author has made additional changes,
follow-up reviews should go back to the same reviewer. This helps build a shared
context and conversation between author and reviewer.
As a team we aim to keep the number of inflight pull requests to a minimum to ensure
that ongoing work is finished before starting new work.
Performing a review
-------------------
To communicate to the rest of the team the status of each pull request, team
members should do the following:
* Assign themselves to the pull request (they should be left assigned to the
pull request until it is merged, closed, or are no longer the reviewer);
* Review the pull request by leaving comments, questions, and suggestions;
* Mark the pull request appropriately (as needing changes or accepted).
If you are unsure about a particular part of the pull request (or are not confident
in your understanding of part of the code) then ask questions or request review
from the team again. When requesting review from the team be sure to leave a comment
with the rationale on why you're putting it back in the queue.