PR Review HOWTO

This document attempts to capture some of the points to consider when reviewing a PR to ensure code quality is kept as high as possible.

High level considerations

• Security:

  • Could the change introduce a potential issue? What additional checks would resolve it?

• Reliability:

  • What happens on failure? (Consider every possible failure)

• Performance:

  • Will this change negatively impact performance?
  • Can any calls block? If so, there should be a log call before the blocking call explaining what is about to be done.

• Maintenance:

  • How easy would this code be to maintain
    • By an experienced developer?
    • By a developer with none / almost no experience of the codebase?

• Usability:

  • Does the change improve or worsen usabililty?
  • Could it be made any better?

Specifics to consider

• Basics

  • Consider all inputs and outputs.
  • What resources are being used? (memory, disk, etc) Are they released?
  • Are all return values and arguments checked?
  • Are there any magic numbers?

• Layout and formatting:

  • Is formatting and naming consistent?
  • Are there any spelling mistakes ("typos")?
  • Do all new files contain the required licence header?

• Design

  • Could the code be simplified / refactored?
    • Is the code "too clever"? Overly clever code tends to be fragile: reject it unless there is a very good documented reason!
  • Is there a better solution to the problem?
  • Is there any duplication?
  • What assumptions does the code make? Are they valid? Are they documented?
  • Does the code make sense?
    • Could someone with minimal exposure to the codebase guess at what it is doing? (hint: they should be able to).

• Documentation

  • Does this change require an update to the README(s) / architecture docs?
  • Is the code sufficiently well commented?
  • Are all functions documented (or for go are they purpose obvious?)
  • Does the document update need to be applied to any other documents (for example, when a PR updates an installation guide, those same changes may need to be applied to one or more of the other installation guides!)

• Logging and Debugging

  • Could this code be debugged from a logfile if it failed?
  • Are error messages and log calls useful and comprehensive? Ensure sensitive information is not displayed though.

• Testing:

  • Has the code been written to be easy to test?
  • Are there new tests? If not, why?

• Environments

  • Will the code work on all distributions?
  • What environments will this code run in?
  • Which user will the code run as?

• Upgrades

  • If the code logs state, how will upgrades be handled?

Mandatory Checks

• Provide constructive comments on the code.
  • If you like some aspect particularly, say so! 😄
• Ensure all automated checks pass.
• Ensure the PR contains atleast one Fixes #XXX commit message referencing an issue that this PR fixes.