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
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 asn1, 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 dictionarycolors = ['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?