Code Review - Recheck the basics

This section refers to the considerations all changes should pass. Sometimes a developer will skip over one of these considerations because they are occupied with trying to solve “the problem” rather than dealing with the engineering aspect of the change. That’s perfectly fine! It one of the reasons why we have code reviews. As you work through this list during a code review it’s especially important to withold judgement.

CI Passes - automated tests, linter, code coverage, etc

If you’re doing code reviews you should have automated tests, linting and some measure of code coverage applied to your project. If you’re not, you should spend a day or two adding in the basic harness to support them. Then over time you can add in more tests, more linting rules, and improve code coverage.

During the code review, double check that these things have run and passed. If the PR has been stagnant with a failure in one of these areas you could check why it failed. Maybe the code coverage dropped because the PR is removing well tested code? It’s only worth mentioning this to the author if the pull request has been waiting for a review for a few days. You don’t want to ask the author to address something that they already know needs to be addressed. However, sometimes people miss things or forget and in that case a gentle reminder can be useful.

Are the migrations/schema changes condensed?

(This is Django specific) If there are multiple migration files being added in the PR, you should check to see if they can be combined. Sometimes multiple migration files will be necessary to support a zero downtime change, or to add a not nullable column to a busy table. There are also times that a developer may have forgotten a field on a new model and added it later on.

While there are ways to squash migrations later on, it’s a good idea to limit how many migrations you’re creating when you can. When you have a multiple environment configuration (multiple production servers, multiple staging servers, multiple developers), it’s difficult to coordinate squashing migrations.

Are the code removals thorough?

If the PR is removing a field, search the codebase to confirm that they were all removed. Yup, not much to say about this one. Embrace the grind.

Are cross-cutting interface changes updated everywhere?

If a field is renamed, search the codebase to confirm all cases were renamed. Again, this one is tedious and doesn’t require much discussion.