mirror of https://github.com/zulip/zulip.git
docs: Add a page documenting review process for pull requests.
- Move the "Submitting a pull request" and "Stages of a pull request" sections of the contributing guide to a dedicated page. - Add more detail.
This commit is contained in:
Alya Abbott
Tim Abbott
parent
f079dec525
commit
afc016c0f0
103
CONTRIBUTING.md
103
CONTRIBUTING.md
|
|
@ -208,101 +208,16 @@ stream](https://chat.zulip.org/#narrow/stream/101-design) in the [Zulip
|
||||||
development community](https://zulip.com/development-community/)
|
development community](https://zulip.com/development-community/)
|
||||||
|
|
||||||
For more advice, see [What makes a great Zulip
|
For more advice, see [What makes a great Zulip
|
||||||
contributor?](#what-makes-a-great-zulip-contributor)
|
contributor?](#what-makes-a-great-zulip-contributor) below. It's OK if your
|
||||||
below.
|
first issue takes you a while; that's normal! You'll be able to work a lot
|
||||||
|
faster as you build experience.
|
||||||
|
|
||||||
### Submitting a pull request
|
### Submitting a pull request
|
||||||
|
|
||||||
When you believe your code is ready, follow the [guide on how to review
|
See the pull request review
|
||||||
code](https://zulip.readthedocs.io/en/latest/contributing/code-reviewing.html#how-to-review-code)
|
process
|
||||||
to review your own work. You can often find things you missed by taking a step
|
guide for detailed instructions on how to submit a pull request, and information
|
||||||
back to look over your work before asking others to do so. Catching mistakes
|
on the stages of review your PR will go through.
|
||||||
yourself will help your PRs be merged faster, and folks will appreciate the
|
|
||||||
quality and professionalism of your work.
|
|
||||||
|
|
||||||
Then, submit your changes. Carefully reading our [Git guide][git-guide], and in
|
|
||||||
particular the section on [making a pull request][git-guide-make-pr], will help
|
|
||||||
avoid many common mistakes. If any part of your contribution is from someone
|
|
||||||
else (code snippets, images, sounds, or any other copyrightable work, modified
|
|
||||||
or unmodified), be sure to review the instructions on how to [properly
|
|
||||||
attribute][licensing] the work.
|
|
||||||
|
|
||||||
[licensing]: https://zulip.readthedocs.io/en/latest/contributing/licensing.html#contributing-someone-else-s-work
|
|
||||||
|
|
||||||
Once you are satisfied with the quality of your PR, follow the
|
|
||||||
[guidelines on asking for a code
|
|
||||||
review](https://zulip.readthedocs.io/en/latest/contributing/code-reviewing.html#asking-for-a-code-review)
|
|
||||||
to request a review. If you are not sure what's best, simply post a
|
|
||||||
comment on the main GitHub thread for your PR clearly indicating that
|
|
||||||
it is ready for review, and the project maintainers will take a look
|
|
||||||
and follow up with next steps.
|
|
||||||
|
|
||||||
It's OK if your first issue takes you a while; that's normal! You'll be
|
|
||||||
able to work a lot faster as you build experience.
|
|
||||||
|
|
||||||
If it helps your workflow, you can submit your pull request marked as
|
|
||||||
a [draft][github-help-draft-pr] while you're still working on it, and
|
|
||||||
then mark it ready when you think it's time for someone else to review
|
|
||||||
your work.
|
|
||||||
|
|
||||||
[git-guide]: https://zulip.readthedocs.io/en/latest/git/
|
|
||||||
[git-guide-make-pr]: https://zulip.readthedocs.io/en/latest/git/pull-requests.html
|
|
||||||
[github-help-draft-pr]: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/about-pull-requests#draft-pull-requests
|
|
||||||
|
|
||||||
### Stages of a pull request
|
|
||||||
|
|
||||||
Your pull request will likely go through several stages of review.
|
|
||||||
|
|
||||||
1. If your PR makes user-facing changes, the UI and user experience may be
|
|
||||||
reviewed early on, without reference to the code. You will get feedback on
|
|
||||||
any user-facing bugs in the implementation. To minimize the number of review
|
|
||||||
round-trips, make sure to [thoroughly
|
|
||||||
test](https://zulip.readthedocs.io/en/latest/contributing/code-reviewing.html#manual-testing)
|
|
||||||
your own PR prior to asking for review.
|
|
||||||
2. There may be choices made in the implementation that the reviewer
|
|
||||||
will ask you to revisit. This process will go more smoothly if you
|
|
||||||
specifically call attention to the decisions you made while
|
|
||||||
drafting the PR and any points about which you are uncertain. The
|
|
||||||
PR description and comments on your own PR are good ways to do this.
|
|
||||||
3. Oftentimes, seeing an initial implementation will make it clear that the
|
|
||||||
product design for a feature needs to be revised, or that additional changes
|
|
||||||
are needed. The reviewer may therefore ask you to amend or change the
|
|
||||||
implementation. Some changes may be blockers for getting the PR merged, while
|
|
||||||
others may be improvements that can happen afterwards. Feel free to ask if
|
|
||||||
it's unclear which type of feedback you're getting. (Follow-ups can be a
|
|
||||||
great next issue to work on!)
|
|
||||||
4. In addition to any UI/user experience review, all PRs will go through one or
|
|
||||||
more rounds of code review. Your code may initially be [reviewed by other
|
|
||||||
contributors](https://zulip.readthedocs.io/en/latest/contributing/code-reviewing.html).
|
|
||||||
This helps us make good use of project maintainers' time, and helps you make
|
|
||||||
progress on the PR by getting more frequent feedback. A project maintainer
|
|
||||||
may leave a comment asking someone with expertise in the area you're working
|
|
||||||
on to review your work.
|
|
||||||
5. Final code review and integration for server and web app PRs is generally done
|
|
||||||
by `@timabbott`.
|
|
||||||
|
|
||||||
#### How to help move the review process forward
|
|
||||||
|
|
||||||
The key to keeping your review moving through the review process is to:
|
|
||||||
|
|
||||||
- Address _all_ the feedback to the best of your ability.
|
|
||||||
- Make it clear when the requested changes have been made
|
|
||||||
and you believe it's time for another look.
|
|
||||||
- Make it as easy as possible to review the changes you made.
|
|
||||||
|
|
||||||
In order to do this, when you believe you have addressed the previous round of
|
|
||||||
feedback on your PR as best you can, post a comment asking reviewers to take
|
|
||||||
another look. Your comment should make it easy to understand what has been done
|
|
||||||
and what remains by:
|
|
||||||
|
|
||||||
- Summarizing the changes made since the last review you received.
|
|
||||||
- Highlighting remaining questions or decisions, with links to any relevant
|
|
||||||
chat.zulip.org threads.
|
|
||||||
- Providing updated screenshots and information on manual testing if
|
|
||||||
appropriate.
|
|
||||||
|
|
||||||
The easier it is to review your work, the more likely you are to receive quick
|
|
||||||
feedback.
|
|
||||||
|
|
||||||
### Beyond the first issue
|
### Beyond the first issue
|
||||||
|
|
||||||
|
|
@ -348,8 +263,8 @@ labels.
|
||||||
1. **Double-check that you have addressed all the feedback**, including any comments
|
1. **Double-check that you have addressed all the feedback**, including any comments
|
||||||
on [Git commit
|
on [Git commit
|
||||||
discipline](https://zulip.readthedocs.io/en/latest/contributing/commit-discipline.html).
|
discipline](https://zulip.readthedocs.io/en/latest/contributing/commit-discipline.html).
|
||||||
2. If all the feedback has been addressed, did you [leave a
|
2. If all the feedback has been addressed, did you leave a
|
||||||
comment](#how-to-help-move-the-review-process-forward)
|
comment
|
||||||
explaining that you have done so and **requesting another review**? If not,
|
explaining that you have done so and **requesting another review**? If not,
|
||||||
it may not be clear to project maintainers or reviewers that your PR is
|
it may not be clear to project maintainers or reviewers that your PR is
|
||||||
ready for another look.
|
ready for another look.
|
||||||
|
|
|
||||||
|
|
@ -14,6 +14,7 @@ commit-discipline
|
||||||
code-style
|
code-style
|
||||||
reviewable-prs
|
reviewable-prs
|
||||||
code-reviewing
|
code-reviewing
|
||||||
|
review-process
|
||||||
zulipbot-usage
|
zulipbot-usage
|
||||||
bug-reports
|
bug-reports
|
||||||
licensing
|
licensing
|
||||||
|
|
|
||||||
|
|
@ -0,0 +1,178 @@
|
||||||
|
# Pull request review process
|
||||||
|
|
||||||
|
Pull requests submitted to Zulip go through a rigorous review process, which is
|
||||||
|
designed to ensure that we are building a high-quality product with a
|
||||||
|
maintainable code base. This page describes the stages of review your pull
|
||||||
|
request may go through, and offers guidance on how you can help keep your pull
|
||||||
|
request moving along.
|
||||||
|
|
||||||
|
## Prepare your pull request
|
||||||
|
|
||||||
|
Beyond writing the code, you will need to prepare your work to make it as easy
|
||||||
|
as possible for others to review. When you believe your code is ready, follow the [guide on how to review
|
||||||
|
code](../contributing/code-reviewing.md#how-to-review-code)
|
||||||
|
to review your own work. You can often find things you missed by taking a step
|
||||||
|
back to look over your work before asking others to do so. Catching mistakes
|
||||||
|
yourself will help your PRs be merged faster, and folks will appreciate the
|
||||||
|
quality and professionalism of your work.
|
||||||
|
|
||||||
|
Be sure to take a careful look at your commit structure and commit messages, and
|
||||||
|
do your best to follow Zulip's [commit
|
||||||
|
guidelines](../contributing/commit-discipline.md). This makes it much easier for
|
||||||
|
code reviewers to spot bugs, so if your PR does not follow the guidelines,
|
||||||
|
reviewers will ask you to restructure it prior to going through the code.
|
||||||
|
|
||||||
|
## Submit your pull request for review
|
||||||
|
|
||||||
|
If you are new to Git, see our guide on [making a pull
|
||||||
|
request][git-guide-make-pr] for detailed technical instructions on how to submit
|
||||||
|
a pull request. When submitting your PR, you will need to:
|
||||||
|
|
||||||
|
- Clearly describe the work you are submitting. Make sure the pull request
|
||||||
|
template is filled out correctly, and that all the relevant points on the
|
||||||
|
self-review checklist (if the repository has one) have been addressed. See the
|
||||||
|
[reviewable pull requests](../contributing/reviewable-prs.md) guide for
|
||||||
|
advice.
|
||||||
|
|
||||||
|
- If you have a question that you expect to be resolved during the review
|
||||||
|
process, put it in a PR comment attached to a relevant part of the changes.
|
||||||
|
The review process will go a lot more smoothly if points of uncertainty
|
||||||
|
are explicitly laid out.
|
||||||
|
|
||||||
|
- Make sure that the pull request passes all CI tests. You can sometimes
|
||||||
|
request initial feedback if there are open questions that will impact how
|
||||||
|
you update the tests. But in general, maintainers will wait for your PR to
|
||||||
|
pass tests before reviewing your work.
|
||||||
|
|
||||||
|
If any part of your contribution is from someone else (code
|
||||||
|
snippets, images, sounds, or any other copyrightable work, modified or
|
||||||
|
unmodified), be sure to review the instructions on how to [properly
|
||||||
|
attribute][licensing] the work.
|
||||||
|
|
||||||
|
If your PR was not ready for review when
|
||||||
|
you first posted it (e.g., because it was failing tests, or you
|
||||||
|
weren't done working through the self-review checklist), notify maintainers when
|
||||||
|
you'd like them to take a look by posting a quick "Ready for review!" comment on
|
||||||
|
the main GitHub thread for your PR.
|
||||||
|
|
||||||
|
[git-guide-make-pr]: ../git/pull-requests.md
|
||||||
|
[licensing]: ../contributing/licensing.md
|
||||||
|
|
||||||
|
### Draft pull requests
|
||||||
|
|
||||||
|
If it helps your workflow, you can submit your pull request marked as
|
||||||
|
a [draft][github-help-draft-pr] while you're still working on it. When ready for
|
||||||
|
review:
|
||||||
|
|
||||||
|
1. Make sure your PR is no longer marked as a [draft][github-help-draft-pr], and
|
||||||
|
doesn't have "WIP" in the title.
|
||||||
|
|
||||||
|
1. Post a quick "Ready for review!" comment on the main GitHub thread for your
|
||||||
|
PR.
|
||||||
|
|
||||||
|
[github-help-draft-pr]: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/about-pull-requests#draft-pull-requests
|
||||||
|
|
||||||
|
## Labels for managing the stages of pull request review
|
||||||
|
|
||||||
|
In the Zulip server/web app repository
|
||||||
|
([`zulip/zulip`](https://github.com/zulip/zulip/)), we use GitHub labels to help
|
||||||
|
everyone understand where a pull request is in the review process. These labels
|
||||||
|
are noted below. Each label is removed by the reviewer for that stage when they
|
||||||
|
have no more feedback on the PR and consider it ready for the next stage of
|
||||||
|
review.
|
||||||
|
|
||||||
|
Sometimes, a label may also be removed because significant changes by
|
||||||
|
the contributor are required before the PR ready to be reviewed again. In that
|
||||||
|
case, the contributor should post a comment mentioning the reviewer when the
|
||||||
|
changes have been completed, unless the reviewer requested some other action.
|
||||||
|
|
||||||
|
## Stages of a pull request review
|
||||||
|
|
||||||
|
This section describes the stages of the pull request review process. Each stage
|
||||||
|
may require several rounds of iteration. Don't feel daunted! Not every PR will
|
||||||
|
require all the stages described, and the process often goes quite quickly for
|
||||||
|
smaller changes that are clearly explained.
|
||||||
|
|
||||||
|
1. **Product review.** Oftentimes, seeing an initial implementation will make it
|
||||||
|
clear that the product design for a feature needs to be revised, or that
|
||||||
|
additional changes are needed. The reviewer may therefore ask you to amend or
|
||||||
|
change the implementation. Some changes may be blockers for getting the PR
|
||||||
|
merged, while others may be improvements that can happen afterwards. Feel
|
||||||
|
free to ask if it's unclear which type of feedback you're getting.
|
||||||
|
(Follow-ups can be a great next issue to work on!)
|
||||||
|
|
||||||
|
Your PR might be assigned the [product
|
||||||
|
review](https://github.com/zulip/zulip/pulls?q=is%3Aopen+is%3Apr+label%3A%22product+review%22)
|
||||||
|
label at this stage, or later in the review process as questions come up. You
|
||||||
|
can also add this label yourself if appropriate. If doing so, be sure to
|
||||||
|
clearly outline the product questions that need to be addressed.
|
||||||
|
|
||||||
|
2. **QA.** If your PR makes user-facing changes, it may get a round of testing
|
||||||
|
without reference to the code. You will get feedback on any user-facing bugs
|
||||||
|
in the implementation. To minimize the number of review round-trips, make
|
||||||
|
sure to [thoroughly test](../contributing/code-reviewing.md#manual-testing)
|
||||||
|
your own PR prior to asking for review.
|
||||||
|
|
||||||
|
Your PR might be assigned the [QA
|
||||||
|
needed](https://github.com/zulip/zulip/pulls?q=is%3Aopen+is%3Apr+label%3A%22QA+needed%22)
|
||||||
|
label at this stage, or later on if re-testing is required.
|
||||||
|
|
||||||
|
3. **Initial code review.** All PRs will go through one or more rounds of code
|
||||||
|
review. Your code may initially be [reviewed by other
|
||||||
|
contributors](../contributing/code-reviewing.md). This helps us make good use
|
||||||
|
of project maintainers' time, and helps you make progress on the PR by
|
||||||
|
getting quick feedback. A project maintainer may leave a comment asking
|
||||||
|
someone with expertise in the area you're working on to review your work.
|
||||||
|
|
||||||
|
4. **Maintainer code review.** In this phase, a Zulip maintainer will do a
|
||||||
|
thorough review of your proposed code changes. Your PR may be assigned the
|
||||||
|
[maintainer
|
||||||
|
review](https://github.com/zulip/zulip/pulls?q=is%3Aopen+is%3Apr+label%3A%22maintainer+review%22)
|
||||||
|
label at this stage.
|
||||||
|
|
||||||
|
5. **Documentation review.** If your PR includes documentation changes, those
|
||||||
|
changes will require review. This generally happens fairly late in the review
|
||||||
|
process, once the UI and the code are unlikely to undergo major changes.
|
||||||
|
Maintainers may indicate that a PR is ready for documentation review by
|
||||||
|
adding a [help center
|
||||||
|
review](https://github.com/zulip/zulip/pulls?q=is%3Aopen+is%3Apr+label%3A%22help+center+review%22)
|
||||||
|
and/or [api docs
|
||||||
|
review](https://github.com/zulip/zulip/pulls?q=is%3Aopen+is%3Apr+label%3A%22api+docs+review%22)
|
||||||
|
label, and mentioning a documentation maintainer in the comments.
|
||||||
|
|
||||||
|
6. **Integration review**. This is the final round of the review process,
|
||||||
|
generally done by `@timabbott` for server and web app PRs. A maintainer will
|
||||||
|
usually assign the [integration
|
||||||
|
review](https://github.com/zulip/zulip/pulls?q=is%3Aopen+is%3Apr+label%3A%22integration+review%22)
|
||||||
|
label when the PR is ready for this phase.
|
||||||
|
|
||||||
|
## How to help move the review process forward
|
||||||
|
|
||||||
|
If there are no comments on your PR for a week after you submit it, you can
|
||||||
|
check again to make sure that it's ready for review, and then post a quick
|
||||||
|
comment to remind Zulip's maintainers to take a look at your work. Consider also
|
||||||
|
[asking for a
|
||||||
|
review](../contributing/code-reviewing.md#asking-for-a-code-review) in the Zulip
|
||||||
|
development community.
|
||||||
|
|
||||||
|
After that, the key to keeping your review moving through the review process is to:
|
||||||
|
|
||||||
|
- Address _all_ the feedback to the best of your ability.
|
||||||
|
- Make it clear when the requested changes have been made
|
||||||
|
and you believe it's time for another look.
|
||||||
|
- Make it as easy as possible to review the changes you made.
|
||||||
|
|
||||||
|
In order to do this, when you believe you have addressed the previous round of
|
||||||
|
feedback on your PR as best you can, post a comment asking reviewers to take
|
||||||
|
another look. Your comment should make it easy to understand what has been done
|
||||||
|
and what remains by:
|
||||||
|
|
||||||
|
- Summarizing the changes made since the last review you received.
|
||||||
|
- Highlighting remaining questions or decisions, with links to any relevant
|
||||||
|
threads in the [Zulip development
|
||||||
|
community](https://zulip.com/development-community/).
|
||||||
|
- Providing updated screenshots and information on manual testing if
|
||||||
|
appropriate.
|
||||||
|
|
||||||
|
The easier it is to review your work, the more likely you are to receive quick
|
||||||
|
feedback.
|
||||||
Loading…
Reference in New Issue