Prior to submitting a merge request be sure the code follows the guides indicated below. The automated formatting and linting tools are strongly recommended to be sure the code conforms and has been vetted.
-
Line width 88 Characters - SHOULD not a MUST
-
flake8 code linter with the flake8-bugbear plugin
For in-depth description of the process we are following please see the referenced documents below, but a quick summary of what is being examined (this list has been extracted from the referenced documentation):
-
The code is well-designed.
-
The functionality is good for the users of the code.
-
Any UI changes are sensible and look good.
-
Any parallel programming is done safely.
-
The code isn’t more complex than it needs to be.
-
The developer isn’t implementing things they might need in the future but don’t know they need now.
-
Code has appropriate unit tests.
-
Tests are well-designed.
-
The developer used clear names for everything.
-
Comments are clear and useful, and mostly explain why instead of what.
-
Code is appropriately documented.
-
The code conforms to the style guides (see the section above).
-
Google Engineering Practices Documentation - Top-level document that links out to a Code Review Developer Guide. In this guide, CL = “changelist”.
-
Google’s How to do a code review - This is a detailed guide on the various aspects and considerations of a code review. This is a good starting point for our team. Note that any references to a style guide point to Google’s style guides, those should be substituted for our team’s styles.
-
Google’s CL author’s guide to getting through code review - A guide with tips and suggestions for structuring and documenting your pull requests. Again, these are good general ideas for how to make pull requests manageable. The concept of making changes small is particularly important, as it can be very time consuming to review a large number of changes.