Pull Request Best Practices
Why Are PRs Important?
- Incredibly valuable feedback mechanism
- Great opportunity to catch bugs early on
- Form of documentation and record-keeping
- Natural forum for discussion
Writing PRs: Always Link Relevant Issue
- Allows the reviewer to access the story to validate if all requirements/acceptance criteria are being met.
- If you install the Linkify JIRA Issues Chrome extension and mention the issue in the title, it will auto link back to the JIRA issue
![](https://s3.amazonaws.com/media-p.slid.es/uploads/161424/images/3903391/pasted-from-clipboard.png)
Writing PRs: Include Details in the Decsription
- Include as many details as possible in the description field, such as setup steps, your thought process, PR dependencies from other repos, edge cases to check, etc...
- Add a "DO NOT MERGE" label if you need the PR to be reviewed but not merged...otherwise assume it will get merged ASAP
![](https://s3.amazonaws.com/media-p.slid.es/uploads/161424/images/3903537/Screen_Shot_2017-06-13_at_11.06.28_AM.png)
Writing PRs: QA Your Stuff
- Before submitting the PR, make sure you QA the topic branch locally.
- For UI-related PRs, this includes testing this like responsiveness
- As engineers, it is our responsibility to ensure we have properly accounted for as many edge cases as possible.
- QA should be considered a last line of defense to account for the most obscure of edge cases.
Writing PRs: Screenshots or GIFs for UI PRs
- For UI related PRs, always include a GIFy or screenshot
- This makes it easy for the reviewer to know where and what to look for when QAing the PR
- Allows tech lead & product manager to get a quick glimpse at the feature before it lands in sandbox
![](https://s3.amazonaws.com/media-p.slid.es/uploads/161424/images/3903426/pasted-from-clipboard.png)
Writing PRs: Utilize Assignee Feature
- Use the "Assignee" section in Github to tag who should take action next
- For Example:
- I tag the reviewer as the Assignee
- Reviewer gives their feedback, removes themselve as Assignee, and tags the writer as the Assignee so they can incorporate any feedback
- Repeat as needed
- The "Reviewer" section should always be whatever reviewers should be on the PR
Writing PRs: Include Documentation
- Always include documentation/comments where appropriate
- Complex sections of code should always include clarifying comments
- Any app/library READMEs or scripts should include full setup instructions, command-line task descriptions, and API documentation
- Always assume the person coming into this code or repo has no idea how it works
![](https://s3.amazonaws.com/media-p.slid.es/uploads/161424/images/3903868/Screen_Shot_2017-06-13_at_12.29.34_PM.png)
Writing PRs: Other Tidbits
- Give descriptive commit messages and titles
- Commit messages should be phrased in the form of what the commit does
- "Changes the button hover color to red"
- "Adds the DELETE /event endpoint for soft-deleting events"
Reviewing PRs: Read the Code!
- No matter how small the PR, resist the temptation to simply gloss over it and give it a "LGTM "
- Do not assume a PR from a more senior team member is solid and does not require an in-depth review
- "Your team is only as good as your weakest reviewer" - Joel Kemp
![](https://s3.amazonaws.com/media-p.slid.es/uploads/161424/images/3903621/Screen_Shot_2017-06-13_at_11.23.52_AM.png)
Reviewing PRs: Give Contextual References
- Where appropriate, give an example code snippet or link a relevant article/tutorial
- These help provide clarity + encourage the engineer learn and apply the knowledge on their own
- If possible, try not to solve the problem for them...guide them to discovering a solution
![](https://s3.amazonaws.com/media-p.slid.es/uploads/161424/images/3903712/Screen_Shot_2017-06-13_at_11.42.01_AM.png)
![](https://s3.amazonaws.com/media-p.slid.es/uploads/161424/images/3903734/Screen_Shot_2017-06-13_at_11.47.24_AM.png)
Reviewing PRs: Check for Test Coverage
- Where appropriate, make sure that the PR has the appropriate amount of test coverage
- Make sure that the tests are good tests that actually validate whether or not the functionality works + accounts for edge cases
- Make sure the tests can be run in isolation from one another! The setup/teardown process of one test should not affect the results of other tests
Reviewing PRs: Show Respect
- Always remember to treat others the way you would want to be treated
- Try to refrain from using a condescending or confrontational tone
- Give compliments!
![](https://s3.amazonaws.com/media-p.slid.es/uploads/161424/images/3903755/Screen_Shot_2017-06-13_at_11.54.37_AM.png)
Reviewing PRs: Other Tidbits
- Review CSS/HTML just as you would JS
- If you are assigned a reviewer, go ahead and QA the topic branch locally
- Always be thinking about edge cases when reviewing/QAing
- Make sure there is proper documentation/comments
Other Resources
Pull Request Best Practices
By Rohit Kalkur
Pull Request Best Practices
Best Practices for Reading/Writing Pull Requests
- 2,371