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

Pull Request Best Practices

By Rohit Kalkur

Pull Request Best Practices

  • 139
Loading comments...

More from Rohit Kalkur