Ctrl+K

Reviewer Guidelines

Reviewer Guidelines#

How to review a pull request#

  • Be friendly

  • Read and follow the Reviewer Checklist

  • Decide how much time you can spare and the detail you can work in. Tell the author!

  • Use the comment/chat functionality within GitHub’s pull requests - it’s useful to have an archive of discussions and the decisions made.

  • Fix the big things first! If there are more important issues, not every style guide has to be stuck to,
    not every slight increase in speed needs to be pointed out, and test coverage doesn’t have to be 100%.

  • Make it clear when a change is optional, or is a matter of opinion

At a minimum

  • Make sure unit and integration tests are passing.

  • (For complete modules) Run the tutorial on your local machine and check it does what it says it does

  • Check everything is fully documented

At least one reviewer needs to

  • Review all the changes in the pull request. Read what it’s supposed to do, check it does that, and make sure the logic is sound.

  • Check that the code follows the CLIMADA style guidelines

  • CLIMADA coding conventions

  • Python Dos and Don’t

  • Python performance tips and best practice for CLIMADA developers

  • If the code is implementing an algorithm it should be referenced in the documentation. Check it’s implemented correctly.

  • Try to think of edge cases and ways the code could break. See if there’s appropriate error handling in cases where the function might behave unexpectedly.

  • (Optional) suggest easy ways to speed up the code, and more elegant ways to achieve the same goal.

There are a few ways to suggest changes

  • As questions and comments on the pull request page

  • As code suggestions (max a few lines) in the code review tools on GitHub. The author can then approve and commit the changes from GitHub pull request page. This is great for typos and little stylistic changes.

  • If you decide to help the author with changes, you can either push them to the same branch, or create a new branch and make a pull request with the changes back into the branch you’re reviewing. This lets the author review it and merge.

Reviewer Checklist#

  • The code must be readable without extra effort from your part. The code should be easily readable (for infos e.g. here)

  • Include references to the used algorithms in the docstring

  • If the algorithm is new, please include a description in the docstring, or be sure to include a reference as soon as you publish the work

  • Variable names should be chosen to be clear. Avoid item, element, var, list, data etc… A good variable name makes it immediately clear what it contains.

  • Avoid as much as possible hard-coded indices for list (no x = l[0], y = l[1]). Rather, use tuple unpacking (see here). Note that tuple unpacking can also be used to update variables. For example, the Fibonacci sequence next number pair can be written as n1, n2 = n2, n1+n2.

  • Do not use mutable (lists, dictionaries, …) as default values for functions and methods. Do not write:

    def function(default=[]):

    but use

    def function(default=None):
    if default is None: default=[]

  • Use pythonic loops, list comprehensions

  • Make sure the unit tests are testing all the lines of the code. Do not only check for working cases, but also the most common wrong use cases.

  • Check the docstrings (Do they follow the Numpydoc conventions, is everything clearly explained, are the default values given and is it clear why they are set to this value)

  • Keep the code simple. Avoid using complex Python functionalities whose use is opaque to non-expert developers unless necessary. For example, the @staticmethod decorator should only be used if really necessary. Another example, for counting the dictionary colors = ['red', 'green', 'red', 'blue', 'green', 'red'], version:

    d = {}
for color in colors:
         d[color] = d.get(color, 0) + 1

    is perfectly fine, no need to complicate it to a maybe more pythonic version

    d = collections.defaultdict(int)
for color in colors:
    d[color] += 1

  • Did the code writer perform a static code analysis? Does the code respect Pep8 (see also the pylint config file)?

  • Did the code writer perform a profiling and checked that there are no obviously inefficient (computation time-wise and memory-wise) parts in the code?

Contents