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

Other Resources

• Thoughtbot PR Review Guidelines
• Github PR Review Guidelines
• Top Ten PR Review Mistakes
• Giving Better Code Reviews