Pull Request Checklist

Hello there 👋 I've created an ultimate pull request checklist which summarises some of the best practises we pursue in every pull request.

There are many blogposts about merge review checklists, but none of them covered everything I wanted so I've created my own.

Authors should memorise it and apply these rules when making a pull request. Otherwise you'll burn reviewer's time and later your time fixing the threads (that you could prevent).

We are using Gitlab for vcs, Django on BE and React w/ typescript on FE. So few points may be related to them.

There it is:

---

Guide to merge requests

• Review your code outside the editor you've written the code in and review the code once more in VCS.
• Have a clear understanding of the problem and set the expectations on what it should do and how before you start to code. Evaluate this once more when you do a self-review.
• You should be able to draw a simple diagram and explain your solution to some colleague. This will help you find any possible problems before you start to code.

Code & Design

• Does it do what it is supposed to do?
• Did you remove everything that is not supposed to be there?
• Aren't there any debug prints or TODO comments you want to do or remove?
• Is the branch up-to-date with master?
• Isn't there any conceivable way this could break other parts of the system?
• Check all of the possible edge cases and have a solution for them.
• Is the code easy to read and understand? Did you choose the best names and organise things as cleanly as possible?
  • name should not involve any historical context
  • name should be clear to any developer who passes by or sees the code for the first time
• Isn't the functionality you've added already implemented somewhere else? If it's duplicated, reconsider the design.
  • if you cannot figure out how to deduplicate the code, try drawing a diagram of the system and find the pure underlying abstraction - there is always an answer
• Is there a single source of truth to every piece of logic?
• Ask yourself - How much expertise and context will anybody reading this code need to understand?
• Did you introduce any new abstractions?
  • Don't they cause additional complexity that can be achieved without the abstraction?
  • Are you able to draw a simple diagram of the system?
  • Did you ever see similar abstraction in some other software?
  • Is it as simple as possible?
  • Will it be easy to expand with more features? Have you thought about what other features we could add there and if it's possible?
• Is there anything you are not sure about? Just ask.

Frontend

• Did you use already implemented ui components?
• Is UI visually consistent? (font size, line height, margin, padding)
• Can you find similar UI in other software?
  • Our's shouldn't be different, else explain why it is so.
• If you added any complex multi-step flow, then you should be able to draw a diagram of it and explain it including possible branches and error validation.
• Does it look OK on phone?
• How is the copy? Are sentences well-formed and clear? Any spelling error or typos?
• What if the request to the backend fails?
• Did you make any changes to the component in the ui or components? Check if all usages of the component still work as expected.

Backend

• Aren't there any possible security threats?
• Isn't any private data exposed to the public? Is the public part of the app available to the public?
• Aren't there any circular module dependencies?
• Did you add any inline import?
  • Why it's so? Are you able to draw a simple diagram of the system and explain?
• Models should not have any methods which doesn't really belong there.
• Are there any issues that will become problems in the next months or years?
  • Thing them out and discuss them with the reviewer in the merge request.
• Does the new classes or modules have only single responsibility?
• Is it possible to write tests for this functionality?
• What if anything fails? Prevent side-effects before changes are committed.

Migrations checklist

Understand that a migration can corrupt data and it is extremely difficult (sometimes impossible) to fix it. A single wrong migration can result in several man-days burnt in vain. We've been through this. It's not nice. Be careful.

• Migrations should be always, reversible. When it's data transformation, you must write reverse transformation.
• Never delete a column that was previously used, instead keep the field in the model and mark it with comment # DEPRECATED in version vX.Y.Z
• Never delete data that is used – if you need to transform data, create a new column and do the transformation there, keep the original data in the original column, use deprecation comment or prepend old_ to the column
• If you have experimented with database schema before settling for a final solution, squash your migrations
• Be extra careful about migrating large tables – there may be millions of rows in that tables and not all operations are safe to run, because it would take too long and fail
  • in that case, create async task which should be run after the code is deployed. Pay attention on that the task should be resumable, because it can be interrupted by failure or another deploy.
• Always use queryset.iterator(), which will prevent memory issues
• Log migration progress by step of reasonable size
• When doing data transformation, create some sample data, so you can correctly handle the edge cases

Peer review

• Use this chrome extension to easily see unresolved threads on merge requests. (Gitlab only)
• Check all of the points above and create merge request threads for any violations.
• Merge request threads should not be resolved without any comment, because then the reviewer needs to check the resolution by himself and it's a waste of time.
• You should ask the reviewer to review your merge request, it will be merged faster. If it takes too long, the author will lose context of the issue and it will take more time to him and the reviewer fix and review it again.

---

Feedback is much appreciated.

Comments

[Troy Witthoeft]

Doing trunk-based development, it's rare that I see a PR large enough to require this type of checklist, but I am definitely going to bookmark this and use it to have better PR sessions. Thank you!