From bddd85c057dce335ade540934cd245e7a1dc5765 Mon Sep 17 00:00:00 2001 From: Josh Kasten Date: Mon, 18 Oct 2021 16:04:40 -0700 Subject: [PATCH] PR template to provide more consistent PRs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A PR template to provide consistent pull requests. This make it easier for the author to know what is required from them while also making it faster for others to understand the code changes while lower the number of incorrect assumptions. PRs should have just enough detail where it is most time efficient for both author(s) and reviewer(s). Explain the motivation, scope, testing done, and any background context around the code change. Goal is to provide a framework for consistently in PRs to catch important mistakes while minimizing friction. This starts with a section that summaries what they will need to do to fill out the template. Below I'll explain the reasoning behind each section. One Line Summary This should be included on every PR. Reviewer gets a high level understanding before they have to dive to deep into details or code. This may be the only section for some small PRs where it can cover everything to understand the change. Motivation Why is this code change being made? Some are very straightforward such as bug fixes and lint rules and adding this section could be redundant. A PR moving code to a background thread for example probably should explain why that is important. Scope Explain what is intended to be effected, and what is known not to change, if acceptable. This gives the reviewer an idea of how far reaching the changes are, and from that they can review for unintended changes. It would be also good to note here if the PR depends on or related code changes not in this PR (such as backend changes). Background context Provide details or links to any research you may have done that lead you to the code you implemented. Or any pre-existing knowledge you have that reviewer may not have or may not be common knowledge to anyone who may look back on your PR in the future. Testing The author of the code should be responsible for testing, however it is ok to ask others for help testing for larger or higher risk PRs. Review your own PR first Give your PR a review yourself first. It might be the end of the day and you just want to submit your PR but a quick self review of your code and PR description can catch a number of silly mistakes. It is ok to request a review for a WIP (work in progress) PR but make sure you are very clear what you would like feedback from the reviewer and what they should skip. Does one thing If it is hard to explain how any codes changes are related to each other in your PR then it most likely needs to be more than one PR. The smaller the PR, the quicker it can be reviewed and the less chance there will be feedback to address. On the flip side a ton of small PRs might be more context switching and overhead. Some overlap that is ok for example is refactoring most of the code in a class so you also applied new styles and naming conventions. Continuing this example, it is also acceptable to also make bug fixes to the class in the PR, however it should be done in its own commit so the reviewer can easily spot any logic changes from non-logic changes. Something that shouldn’t be in the same PR for example is a fixing a bug and also updating project settings just because they were out of date. Review for unintended changes To the best of the reviewer’s ability they should review the code for any unintended changes or bugs. Ask the PR author questions and to add more background context to the PR itself too if you think it will be helpful to others. It might also be good to get a 2nd reviewer’s eyes on the PR too if the changes are high risk or not something the reviewer feels they understand well enough. Review for readability It is important that code is as easy to follow and understand so the reviewer has as easy of a time as possible to spot any bugs. Also readability helps make future changes and understanding easier outside of review process. Explain what is confusion about the code, optionally suggest possible changes to help the author. The author should attempt to improve readability in the following order; simplify with less code, followed by splitting up code into well named functions and variables, followed by adding comments to the code. Explaining only in replies on the PR is the least preferred since it is the hardest to discover, but still better than in slack or in person where the reasoning behind the code can be lost forever. Public APIs changes Any changes that are designed to be used outside of the repo should have had some other documentation or discussion around them before the PR was created. The reviewer should check for unintended API changes, mistakes on intended API changes, and as a last chance to discusses the API design. Code style Code style should ideally be automated as part of a CI process so a human doesn’t have to spend time reviewing. However if not in place or the lint tools can’t verify some of it then we should review for it. Turn around It should be high priority for the reviewer and author to complete the PR, and to know who is waiting on who. Follow ups should be quick enough where the context spin up / switching should be little to none for both the reviewer and the author. If your WIP PR is becoming large ask yourself if splitting the PR up into different PRs makes sense, this way reviewing could even be parallelized by multiple people. If it doesn’t make sense to split up the PR it might be a good idea to ask someone to review while in WIP status if you can define what should and should not be reviewed. This template is designed for everyone who makes a PR to this repo as well as those who review them. --- .github/pull_request_template.md | 60 ++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) create mode 100644 .github/pull_request_template.md diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md new file mode 100644 index 0000000000..524e758e45 --- /dev/null +++ b/.github/pull_request_template.md @@ -0,0 +1,60 @@ + +# READ AND DELETE THIS SECTION BEFORE SUBMITTING PR +* **Fill out each _REQUIRED_ section** +* **Fill out _OPTIONAL_ sections, remove section if it doesn't apply to your PR** +* **Read and fill out each of the checklists below** +* **Remove this section after reading** + + +# Description +## One Line Summary +**REQUIRED** - Very short description that summaries the changes in this PR. + +## Details + +### Motivation +**REQUIRED -** Why is this code change being made? Or what is the goal of this PR? Examples: Fixes a specific bug, provides additional logging to debug future issues, feature to allow X. + +### Scope +**RECOMMEND - OPTIONAL -** What is intended to be effected. What is known not to change. Example: Notifications are grouped when parameter X is set, not enabled by default. + +### OPTIONAL - Other +**OPTIONAL -** Feel free to add any other sections or sub-sections that can explain your PR better. + +# Testing +## Unit testing +**OPTIONAL -** Explain unit tests added, if not clear in the code. + +## Manual testing +**RECOMMEND - OPTIONAL -** Explain what scenarios were tested and the environment. +Example: Tested opening a notification while the app was foregrounded, app build with Android Studio 2020.3 with a fresh install of the OneSignal example app on a Pixel 6 with Android 12. + +# Affected code checklist + - [ ] Notifications + - [ ] Display + - [ ] Open + - [ ] Push Processing + - [ ] Confirm Deliveries + - [ ] Outcomes + - [ ] Sessions + - [ ] In-App Messaging + - [ ] REST API requests + - [ ] Public API changes + +# Checklist +## Overview + - [ ] I have filled out all **REQUIRED** sections above + - [ ] PR does one thing + - If it is hard to explain how any codes changes are related to each other then it most likely needs to be more than one PR + - [ ] Any Public API changes are explained in the PR details and conform to existing APIs + +## Testing + - [ ] I have included test coverage for these changes, or explained why they are not needed + - [ ] All automated tests pass, or I explained why that is not possible + - [ ] I have personally tested this on my device, or explained why that is not possible + +## Final pass + - [ ] Code is as readable as possible. + - Simplify with less code, followed by splitting up code into well named functions and variables, followed by adding comments to the code. + - [ ] I have reviewed this PR myself, ensuring it meets each checklist item + - WIP (Work In Progress) is ok, but explain what is still in progress and what you would like feedback on. Start the PR title with "WIP" to indicate this.