From afc016c0f0ce5673c88af26be7e233332690e9db Mon Sep 17 00:00:00 2001 From: Alya Abbott Date: Wed, 24 May 2023 00:15:57 -0700 Subject: [PATCH] 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. --- CONTRIBUTING.md | 103 ++-------------- docs/contributing/index.md | 1 + docs/contributing/review-process.md | 178 ++++++++++++++++++++++++++++ 3 files changed, 188 insertions(+), 94 deletions(-) create mode 100644 docs/contributing/review-process.md diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index d99b283740..b4e1492c63 100644 --- a/CONTRIBUTING.md +++ b/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/) For more advice, see [What makes a great Zulip -contributor?](#what-makes-a-great-zulip-contributor) -below. +contributor?](#what-makes-a-great-zulip-contributor) below. 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. ### Submitting a pull request -When you believe your code is ready, follow the [guide on how to review -code](https://zulip.readthedocs.io/en/latest/contributing/code-reviewing.html#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. - -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. +See the pull request review +process +guide for detailed instructions on how to submit a pull request, and information +on the stages of review your PR will go through. ### Beyond the first issue @@ -348,8 +263,8 @@ labels. 1. **Double-check that you have addressed all the feedback**, including any comments on [Git commit discipline](https://zulip.readthedocs.io/en/latest/contributing/commit-discipline.html). - 2. If all the feedback has been addressed, did you [leave a - comment](#how-to-help-move-the-review-process-forward) + 2. If all the feedback has been addressed, did you leave a + comment 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 ready for another look. diff --git a/docs/contributing/index.md b/docs/contributing/index.md index 8178016c96..60d42f7455 100644 --- a/docs/contributing/index.md +++ b/docs/contributing/index.md @@ -14,6 +14,7 @@ commit-discipline code-style reviewable-prs code-reviewing +review-process zulipbot-usage bug-reports licensing diff --git a/docs/contributing/review-process.md b/docs/contributing/review-process.md new file mode 100644 index 0000000000..f887f1d5ad --- /dev/null +++ b/docs/contributing/review-process.md @@ -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.