Skip to content

review a pull request

Jump to bottom
Todd Tyree edited this page Jul 16, 2018 · 8 revisions

Review a Pull Request

There are a number of different ways to do this. However, this is the way we want it done in Platform:

  1. Are there tests? If so, do they pass?
    • Don't review anything that has failing tests-ask the PR owner to fix the tests first.
    • If not, why not?
  2. Does the PR have a description?
    • PRs should always have a description summarising its context. Why has the PR been raised and what issue does it address?
  3. Is there an issue for this PR in Github?
    • Generally speaking, PRs should always be linked to specific issues. The issue should be linked or mentioned in the description.
    • Does the PR do what is specified in the issue?
    • Does the issue have a checklist? If so, has the owner of the PR checked all the items? The relevant items if the PR does not close the issue?
  4. Can you undertand what the code is doing without having it explained?
    • Declarative code in discrete, organised units is best. However, you can also use comments and commit messages.
      • Well-named variables are a common part of declarative code. Do the variable names all give you an idea of what they do?
    • If you cannot understand it, start a conversation on the PR about what you do not understand. Be concise about what you don't understand. Try to link your comments to specific lines of code so that updates to the PR close them.
  5. Does the code have any obvious howlers?
    • We all make mistakes and miss things. We all ignore old bits of bad code when we're in a hurry. If you notice anything like this in code you're reviewing, ask the PR issuer to fix it.
    • If it's something old that was there before they started and they ask if they can skip it, ask them to open an issue to fix it later.
  6. Give kudos for the good things you see in a PR as well.
    • A 👍 or encoraging remark on something well done goes a long way.
  7. Don't sweat the implementation details.
    • If the code works, is clear and conforms to standards of style and consistency don't spend a lot of time asking for changes.
Clone this wiki locally