Guidelines I follow to create merge requests (as GitLab calls them) and pull requests (as GitHub calls them). I’ll use code diffs on this page to save on characters.

Note that these are guidelines and not hard rules and it’s an important skill to know when to go bend or break rules.

Show what’s changed

Depending on who is reviewing your code diff, they might not be familiar with the area of the codebase you are changing. I have a strong preference for including screenshots whenever we are making changes to the UI. I also find that a table with Before and After does wonders for guiding the attention of the reviewer. For backend or database changes we can do screenshots of terminal outputs or paste in the terminal output in the description - your code does something and show me what that something is.

Reproduce steps

Especially relevant for fixing bugs or if your reviewer likes to run functionality locally as part of their review workflow describing the steps for testing locally saves on back and forth.

Why?

Describe why we are making these changes. Often times this is a link to an issue but it’s easier on the reviewer if you give a short paragraph in your description as well as a link to the larger story.

Tests are included in the code diff

Don’t tell the reviewer that “I will add tests in the future”. Tests will pressure test your implementation and they also serve as documentation of how the implementation is meant to be used.

A notable exception for skipping tests is if production is on fire and your change is to put out the fire.

Single purpose code diffs

The more focused your code diff is, the easier it is to review and the more effective your change is. Don’t take this too literally and make atomic micro code diffs (5 typos in the documentation can be addressed in 1 code diff) but it’s good to be mindful of not introducing 5 complex components at once.

Be mindful of size

If I need to review thousands of lines of code it’s going to take me a while to build context on what’s changing. Depending on my work schedule I might also need a meaningful break in meetings before I can commit to even starting to read the code diff. With the increased surface area there’s also more ripple effects to be mindful. Additionally I am more likely to miss some detail.

So time to review increases and quality of review goes down.

That said, sometimes it does make sense to do a huge code diff instead of trickling changes over multiple weeks. If you have automatic code fixes you trust (eslint or codemods) it can make sense to get it over with quickly. Or if there’s a god class that needs to be refactored. In this case it’s important that you get a willing coworker to agree to review before you dump a 10k code diff in their inbox.

Make the change safe to merge

Can the database migration be rolled back? Is the code behind a feature flag? Can we hit “Revert” and get back to functional state in case we shipped a bug?

De-risking the change makes it a lot easier to move forward. Also it’s a good practice to think about how we recover.