From a27fae16df89a24f28e568d419e48ddec73d1f45 Mon Sep 17 00:00:00 2001 From: Alya Abbott Date: Fri, 3 May 2024 16:35:15 -0700 Subject: [PATCH] contributor docs: Improve article on submitting a pull request. - Give more guidance on how to think about PRs. - Structure article as series of steps. - Link to other pages to avoid duplicating content. --- docs/contributing/commit-discipline.md | 16 +- docs/contributing/index.md | 2 +- docs/contributing/reviewable-prs.md | 257 +++++++++++++------------ 3 files changed, 149 insertions(+), 126 deletions(-) diff --git a/docs/contributing/commit-discipline.md b/docs/contributing/commit-discipline.md index ca62056025..a4d1df52e0 100644 --- a/docs/contributing/commit-discipline.md +++ b/docs/contributing/commit-discipline.md @@ -7,11 +7,17 @@ makes the commit history a much more useful resource for developers trying to understand why the code works the way it does, which also helps a lot in preventing bugs. +Use `git rebase -i` as much as you need to shape your commit structure. See the +[Git guide](../git/overview.md) for useful resources on mastering Git. + ## Each commit must be coherent - It should pass tests (so test updates needed by a change should be in the same commit as the original change, not a separate "fix the tests that were broken by the last commit" commit). +- It should not make Zulip worse. For example, it is fine to add backend + capabilities without adding a frontend to access them. It's not fine to add a + frontend component with no backend to make it work. - It should be safe to deploy individually, or explain in detail in the commit message as to why it isn't (maybe with a [manual] tag). So implementing a new API endpoint in one commit and then adding the @@ -24,8 +30,14 @@ helps a lot in preventing bugs. ## Commits should generally be minimal -- Significant refactorings should be done in a separate commit from - functional changes. +Whenever possible, find chunks of complexity that you can separate from the +rest of the project. + +- If you need to refactor code, add tests for existing functionality, + rename variables or functions, or make other changes that do not + change the functionality of the product, make those changes into a + series of preparatory commits that can be merged independently of + building the feature itself. - Moving code from one file to another should be done in a separate commits from functional changes or even refactoring within a file. - 2 different refactorings should be done in different commits. diff --git a/docs/contributing/index.md b/docs/contributing/index.md index bda33d22fe..5494066d7e 100644 --- a/docs/contributing/index.md +++ b/docs/contributing/index.md @@ -12,8 +12,8 @@ asking-great-questions design-discussions commit-discipline code-style -reviewable-prs code-reviewing +reviewable-prs review-process zulipbot-usage reporting-bugs diff --git a/docs/contributing/reviewable-prs.md b/docs/contributing/reviewable-prs.md index d2d0fa331c..c2c1260145 100644 --- a/docs/contributing/reviewable-prs.md +++ b/docs/contributing/reviewable-prs.md @@ -1,79 +1,147 @@ # Submitting a pull request -This page offers some tips for making your pull requests easy to review. -Following this advice will help the whole Zulip project move more quickly by -saving maintainers time when they review your code. It will also make a big -difference for getting your work integrated without delay. For a detailed -overview of Zulip's PR review process, see the [pull request review -process](../contributing/review-process.md) guide. +A pull request (PR) is a presentation of your proposed changes to Zulip. Your aim +should be to explain your changes as clearly as possible. This will help +reviewers evaluate whether the proposed changes are correct, and address any +open questions. Clear communication helps the whole Zulip project move more +quickly by saving maintainers time when they review your code. It will also make +a big difference for getting your work integrated without delay. -## Posting a pull request +You will go through the following steps to prepare your work for review. Each +step is described in detail below, with links to additional resources: -- Before requesting a review for your pull request, follow our guide to - carefully [review and test your own - code](./code-reviewing.md#reviewing-your-own-code). Doing so can save many - review round-trips. +1. Write your code [with clarity in mind](#write-clear-code). +1. [Organize your proposed changes](#organize-your-proposed-changes) into a + series of commits that tell the story of how the codebase will change. +1. [Explain your changes](#explain-your-changes) in the description for your + pull request, including [screenshots](#demonstrating-visual-changes) for + visual changes. +1. Carefully [review your own work](#review-your-own-work). +1. [Submit your pull request](#submit-your-pull-request-for-review) for review. -- 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 [pull request review process](../contributing/review-process.md) guide +for a detailed overview of what happens once your pull request is submitted. -- Be sure to explicitly call out any open questions, concerns, or decisions you - are uncertain about. +## Write clear code -## Prepare your pull request +When you write code, you should make sure that you understand _why it works_ as +intended. This is the foundation for being able to explain your proposed changes +to others. -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. +Zulip’s coding philosophy is to focus relentlessly on making the codebase easy +to understand and difficult to make dangerous mistakes. Our linters, tests, code +style guidelines, [testing philosophy](../testing/philosophy.md), [commit +discipline](../contributing/commit-discipline.md), this documentation, and our +attention to detail in [code review](../contributing/review-process.md) are all +essential elements of this strategy. Following these guidelines is essential if +you're like your work to be merged into the project. -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. +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.md) the work. + +## Organize your proposed changes + +The changes you submit will be organized into a series of commits. A PR might +contain a single commit, or a dozen or more, depending on the changes being +made. + +Commits help you tell the story of how each change you are proposing is +necessary or helpful. If you were presenting your changes, a commit might be a +slide in your presentation. As a rough guideline, a good commit usually has less +than 100 lines of code changes. If you can see a way to split a commit into +different pieces of meaning, you should split it. + +Keep in mind that you are presenting your final work product, _not_ the path you +took to get there. You should never have a commit that can be described as +fixing a mistake in an earlier commit in the same PR; use `git rebase -i` to fix +the mistake in the original commit. + +See the [commit discipline guide](../contributing/commit-discipline.md) for more +details on how to structure your commits, and guidelines on how to write good +commit messages. Your pull request can only be reviewed once you've followed +these guidelines to the best of your ability. This makes it much easier for +reviewers to understand your work and identify any problems. + +Ideally, when reviewing a pull request for a complex project, Zulip's +maintainers should be able to verify and merge the first few commits, and leave +comments on the rest. It is by far the most efficient way to do collaborative +development, since one is constantly making progress, we keep branches small, +and reviewers don't end up repeatedly going over the earlier parts of a pull +request. + +## Explain your changes + +By the time you are submitting your pull request, you should already have put a +lot of thought into how to organize and present your proposed changes. In the +description for your pull request, you will: + +- Provide an overview of your changes. +- Note any differences from prior plans (e.g., from the description of the issue you + are solving). +- Call out any open questions, concerns, or decisions you are uncertain about. + The review process will go a lot more smoothly if points of uncertainty are + explicitly laid out. +- Include screenshots for all visual changes, so that they can be reviewed + without running your code. See [below](#demonstrating-visual-changes) for + detailed instructions. + +If you have a question about a specific part of your code that you expect to be +resolved during the review process, put it in a PR comment attached to a +relevant part of the changes. + +Take advantage of [GitHub's formatting][github-syntax] to make your pull request +description and comments easy to read. + +### Discussions in the development community + +Any questions for which broader feedback or visibility is helpful are discussed +in the [Zulip development community](https://zulip.com/development-community/). + +If there has been a conversation in the [Zulip development +community][zulip-dev-community] about the changes you've made or the issue your +pull request addresses, please cross-link between your pull request and those +conversations in both directions. This provides helpful context for maintainers +and reviewers. Specifically, it's best to link from your pull request [to a +specific message][link-to-message], as these links will still work even if the +topic of the conversation is renamed, moved or resolved. + +Once you've created a pull request on GitHub, you can use one of the [custom +linkifiers][dev-community-linkifiers] in the development community to easily +link to your pull request from the relevant conversation. + +## Review your own work + +Before requesting a review for your pull request, follow our [review +guide](./code-reviewing.md#reviewing-your-own-code) to carefully review and test +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 reviewers will appreciate the quality +and professionalism of your work. + +The pull request template in the `zulip/zulip` repository has a checklist of +reminders for points you need to cover in your review. Make sure that all the +relevant items on the self-review checklist have been addressed. ## 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: +request](../git/pull-requests.md) for detailed technical instructions on how to +submit a pull request. -- 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. +When submitting your PR, you will need to 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 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. +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 +clear comment on the main GitHub thread for your PR with details on any changes +from the original version; this is very helpful for any maintainers who already +read the draft PR. -- 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 +## 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 @@ -87,69 +155,7 @@ review: [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 -## Working on larger projects - -For a larger project, aim to create a series of small (less than 100 lines of -code) commits that are each safely mergeable and move you towards your goal. A -mergeable commit: - -- Is well-tested and passes all the tests. That is, changes to tests should be in - the same commit as changes to the code that they are testing. - -- Does not make Zulip worse. For example, it is fine to add backend capabilities - without adding a frontend to access them. It's not fine to add a frontend - component with no backend to make it work. - -Ideally, when reviewing a branch you are working on, the maintainer -should be able to verify and merge the first few commits and leave -comments on the rest. It is by far the most efficient way to do -collaborative development, since one is constantly making progress, we -keep branches small, and developers don't end up repeatedly reviewing -the earlier parts of a pull request. - -Here is some advice on how to proceed: - -- Use `git rebase -i` as much as you need to shape your commit - structure. See the [Git guide](../git/overview.md) for useful - resources on mastering Git. - -- If you need to refactor code, add tests, rename variables, or make - other changes that do not change the functionality of the product, make those - changes into a series of preparatory commits that can be merged independently - of building the feature itself. - -- To figure out what refactoring needs to happen, you might first make a hacky - attempt at hooking together the feature, with reading and print statements as - part of the effort, to identify any refactoring needed or tests you want to - write to help make sure your changes won't break anything important as you work. - Work out a fast and consistent test procedure for how to make sure the - feature is working as planned. - -- Build a mergeable version of the feature on top of those refactorings. - Whenever possible, find chunks of complexity that you can separate from the - rest of the project. - -See our [commit discipline guide](../contributing/commit-discipline.md) for -more details on writing reviewable commits. - -## Tips and best practices - -When writing comments for pull requests, it's good to be familiar with -[GitHub's basic formatting syntax][github-syntax]. Here are some additional -tips and best practices that Zulip contributors and maintainers have found -helpful for writing clear and thorough pull request comments: - -- If there has been a conversation in the [Zulip development - community][zulip-dev-community] about the changes you've made or the issue - your pull request addresses, please cross-link between your pull request and - those conversations. This provides helpful context for maintainers and - reviewers. Specifically, it's best to link from your pull request [to a - specific message][link-to-message], as these links will still work even if the - topic of the conversation is renamed, moved or resolved. - - Once you've created a pull request on GitHub, you can use one of the [custom - linkifiers][dev-community-linkifiers] in the development community to easily - link to your pull request from the relevant conversation. +## Demonstrating visual changes - For [screenshots or screencasts][screenshots-gifs] of changes, putting them in details/summary tags reduces visual clutter @@ -166,6 +172,11 @@ helpful for writing clear and thorough pull request comments: ``` +- Screencasts are difficult to review, so use them only when necessary to + demonstrate an interaction. Keep videos as short as possible. If your changes + can be seen on a screenshot, be sure to include screenshots in addition to any + videos. + - For before and after images or videos of changes, using GithHub's table syntax renders them side-by-side for quick and clear comparison. While this works well for narrow or small images, it can be hard to