review a pull request

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?
   • A PR 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, a PR should always be linked to a specific issue. 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? Has the owner checked 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, commit messages and threads in issues (in that order).
     • Well-named variables are an essential component of declarative code. Do the variable names all give you an idea of what they do?
   • If you cannot understand the code, 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 will 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.
   • A 👍 or encouraging 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.

Commit Messages

There is something of a (simple) art to getting these right. Chris Beam has a great blog post on how to write them. The TL;DR version is:

1. Separate subject from body with a blank line
2. Limit the subject line to 50 characters
3. Capitalize the subject line
4. Do not end the subject line with a period
5. Use the imperative mood in the subject line
6. Wrap the body at 72 characters
7. Use the body to explain what and why vs. how