Reviewing Pull Requests

There's a nice document here giving general considerations for code reviews.

Below is a list of the things that you want to be looking at in your review, so you should be making sure that the proposed changes are acceptable for each criteria:

High level

Questions to think about:

• What is the purpose of this pull request? This is the first thing to understand, as you’re going to be judging everything else against it:
  • Is it functionality that we want?
  • Does it provide the functionality it claims?
  • Does it provide more functionality than it claims? Remember that we try to keep pull requests based around one (or a small number of features)
• Is the code in the right place? i.e. correct, sensibly named, module.
• Does it actually merge cleanly? If not you need get the author to use their git skills to get it to merge cleanly.

Style

Coding style and documentation are important to keep the code base readable and understandable by other people. These are things to look at when reviewing:

• PEP8 compliance. We're a little relaxed about the line length (provided it's not more than ~100 characters), but everything else is pretty rigid. For the author: try using automated tools like pycodestyle, I run it directly in my editor.
• Docstrings. All public functions, classes and methods must have informative and correct docstrings.
• Comments - (note that these are not the same as docstrings, are neither is a substitute for the other). Should allow a fellow developer (like yourself) to understand what and why a section of code is doing. Comments should be correct and up to date (an incorrect comment is worse than no comment at all).
• Variable, class and method names - should be informative and if possible short, the former is more important. Don't shorten names unless it's significant, i.e. frequency -> freq (good); input -> inpt (bad).

Code

Read through the code and try and understand what it is doing. If you can’t figure it out easily then either the code or its comments need to be revised, so ask the author to revise it. The logic should either be simple or it should be well explained in the comments.

Things to look for:

• Could the code be simplified?
• Are there superfluous sections of code that never get called?
• Is there old debug code/print statements that could be deleted?