From 4d5eaea6dc2829bbb52dbb9614f69043e209e1d9 Mon Sep 17 00:00:00 2001 From: Jamey Sharp Date: Wed, 5 Apr 2023 13:47:02 -0700 Subject: [PATCH] Update PR template and dev process docs (#6158) Overall, I'm just trying to make these bits of documentation reflect our process as it stands today. There are some specific changes I want to draw attention to though. Asking new contributors to pick a reviewer is a waste of time for two reasons: Only people with write access to the repository are allowed to pick reviewers, and new contributors have no idea who would be a good reviewer for their PR anyway. So I'm deleting all mention of that. We now auto-assign reviewers instead. By the time someone is opening a PR, asking them to open an issue just makes extra work for everyone. They've already picked an approach without discussing it; we might as well look at what they did. We may then have to ask them to take a different approach, but at that point, asking them to open an issue won't save them any effort. I removed mention of tests from the pull request template. There are many things we'd like to see in a PR, and we may have to ask for them during review if the contributor doesn't follow our development process documentation. But I think the only crucial information for starting a review is the two questions I'm leaving in the template: why do you want this, and where can I find more context? The code of conduct link still had the branch name as `master`, which is a hint at how long it's been since anyone reviewed it. --- .github/pull_request_template.md | 23 ++++++------- docs/contributing-development-process.md | 44 ++++++++++++++---------- 2 files changed, 35 insertions(+), 32 deletions(-) diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 18ad42fa5b..6338718dc1 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -1,18 +1,15 @@ diff --git a/docs/contributing-development-process.md b/docs/contributing-development-process.md index 78781a4b7d..716167edd8 100644 --- a/docs/contributing-development-process.md +++ b/docs/contributing-development-process.md @@ -15,9 +15,8 @@ Our issue templates might help you through the process. ### When submitting PRs - - Please fill in the pull request template as appropriate. It is usually - helpful, it speeds up the review process and helps understanding the changes - brought by the PR. + - Please answer the questions in the pull request template. They are the + minimum information we need to know in order to understand your changes. - Write clear commit messages that start with a one-line summary of the change (and if it's difficult to summarize in one line, consider @@ -26,27 +25,35 @@ Our issue templates might help you through the process. code are affected, which features are affected, and anything that reviewers might want to pay special attention to. - - If there is code which needs explanation, prefer to put the explanation in - a comment in the code, or in documentation, rather than in the commit - message. + - If there is code which needs explanation, prefer to put the explanation in a + comment in the code, or in documentation, rather than in the commit message. + Commit messages should explain why the new version is better than the old. + + - Please include new test cases that cover your changes, if you can. If you're + not sure how to do that, we'll help you during our review process. - For pull requests that fix existing issues, use [issue keywords]. Note that not all pull requests need to have accompanying issues. - - Assign the review to somebody from the [Core Team], either using suggestions - in the list proposed by Github, or somebody else if you have a specific - person in mind. - - When updating your pull request, please make sure to re-request review if the request has been cancelled. ### Focused commits or squashing -We generally squash sequences of incremental-development commits together into -logical commits (though keeping logical commits focused). Developers may do -this themselves before submitting a PR or during the PR process, or Core Team -members may do it when merging a PR. Ideally, the continuous-integration tests -should pass at each logical commit. +We are not picky about how your git commits are structured. When we merge your +PR, we will squash all of your commits into one, so it's okay if you add fixes +in new commits. + +We appreciate it if you can organize your work into separate commits which each +make one focused change, because then we can more easily understand your +changes during review. But we don't require this. + +Once someone has reviewed your PR, it's easier for us if you _don't_ rebase it +when making further changes. Instead, at that point we prefer that you make new +commits on top of the already-reviewed work. + +That said, sometimes we may need to ask you to rebase for various technical +reasons. If you need help doing that, please ask! ### Review and merge @@ -54,10 +61,9 @@ Anyone may submit a pull request, and anyone may comment on or review others' pull requests. However, one review from somebody in the [Core Team] is required before the Core Team merges it. -Even Core Team members should create PRs for every change, including minor work -items (version bump, removing warnings, etc.): this is helpful to keep track of -what has happened on the repository. Very minor changes may be merged without a -review, although it is always preferred to have one. +Even Core Team members must create PRs and get review from another Core Team +member for every change, including minor work items such as version bumps, +removing warnings, etc. [issues]: https://guides.github.com/features/issues/ [pull requests]: https://help.github.com/articles/about-pull-requests/