Generally, we aim for efficiency, and we value trust over enforcement.
That means we mostly use comments and rarely use the request_changes feature in Github,
unless we've identified things that we think will be blockers in the near future if they don't get fixed now.
We respect each other's input and take it seriously, so even if it's only a comment, we will at least respond to every comment, even if we don't implement the suggestions as written.
We agree not to mark comments as resolved unless we've responded on Github, or otherwise addressed them in the code.
If we discussed in Slack, we've written a summary of that discussion in our response.
-
The code meets our basic needs for readability
- Python-specific things:
- pythonic standards are followed
- code is formatted with black
- Docstrings are present on all methods, with a description and parameter definitions
- Type hinting is used for all parameters
- Tests exist for most (ideally all) methods, and demonstrate how the code should work
- Method naming makes sense (consistent, neither too terse nor too verbose)
- Variable naming makes sense (consistent, neither too terse nor too verbose)
- Schema/field names make sense (consistent, neither too terse nor too verbose)
- I've assumed that the author may have made typos, or left stale comments, and checked for that
- Python-specific things:
-
I've checked for obvious structural oversights/mistakes
- Conditionals: if/then logic
- Optional/mutable parameters are handled correctly
- All methods return something (if not part of a class)
- If OOP, inheritance is handled correctly
- It's clear what this code is intended to do, according to the PR description
- I can understand what the code is doing, based on naming, docstrings, and comments
- I can understand how the code works. If it wasn't obvious, I've had the author walk me through it.
- I can understand the choices the author made, if not immediately then after discussion.
- Appropriate charts are provided to illustrate conclusions
- Charts are labeled and scaled according to our basic needs for readability & interpretability
- X and Y axes are labeled, and all charts have titles when faceted Scales are scaled correctly
- If not all the data are visible otherwise, a log scale is used
- If comparing multiple facets, all sub-plots are scaled the same way
- Error bars or confidence intervals are shown whenever possible
- Charts are labeled with how many data points were used (n=xx)
I would add that criticism needs to be constructive. It's important for the code reviewer to somewhat think about the feelings of those who wrote the code. I know this is in the eye of the beholder, but there are lines we can draw up.