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
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
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
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
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
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
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!
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