PR template to provide more consistent PRs #1462
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
jkasten2
force-pushed
the
github/pr_template_create
branch
from
ecf9577
to
bddd85c
Compare
jkasten2
requested review from
adamschlesinger,
emawby,
Jeasmine,
jmadler,
rgomezp and
a team
emawby
approved these changes
Nov 2, 2021
There was a problem hiding this comment.
I'd really like to have an optional section for "potential issues". If the dev is concerned that their solution might cause other problems down the road there should be a block to note that.
Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @Jeasmine, @jmadler, @nan-li, and @rgomezp)
emawby
reviewed
Nov 4, 2021
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @Jeasmine, @jkasten2, @jmadler, @nan-li, and @rgomezp)
a discussion (no related file):
Just a note that GitHub thinks unchecked boxes are incomplete tasks
I noticed this as well, we can ignore this "x of x tasks" status or change all the non-required checkboxes into something else so it should show as all tasks complete if they did it right. I saw this issue on Github feedback we can upvote on too: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
One Line Summary
Provide a Github template and checklist to be filled out by the PR author to describe the PR in an organized way.
Details
Motivation
Goal is to provide a framework for consistency in PRs to catch important mistakes while minimizing friction. PRs that have just enough detail where it is most time efficient for both author(s) and reviewer(s).
Scope
A single PR template to be shown to the author each time they create a new PR through Github. PR author is expected to read and fill out the template. This also is a list of what a reviewer should review for as well.
Background context
Some content was inspired by How to Make Your Code Reviewer Fall in Love with You.
Reasoning for sections
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.
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.
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.
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.
Continuous integration passes
The PR author should make sure the CI is passing. However sometimes tests are flaky and not related to the change. The PR can still be merged if both the author and reviewer agree.
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.
What should be included in the PR details?
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. Start with a one-line summary so the reviewer gets a high level understanding before they have to dive to deep into details or code. We will cover each below.
One-line Summary
This should be included on every PR. It may be the only thing in some cases for 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.
Affected code checklist
Checklist
Overview
Testing
Final pass
This change is