This post was originally written as an internal article on Cheesecake Labs’ wiki. It merges (no pun intended) pull request practices from many different cultures and backgrounds into a definitive workflow. We will discuss why you should have a guide for managing your PR lifecycle, as well as tips on how to accomplish that.
What should you check before opening a pull request?
Before opening a pull request, double check your code and run the tests locally. Reviewing your own diff first catches problems before you take up a reviewer’s time, and running the tests on your machine avoids delaying other people’s work, increasing costs, or sending a bunch of failure emails. git diff is an excellent tool for that and it’s just a few keystrokes away! Try git diff master.. (where master is the branch you’ll target your PR with).
Think before using your CI service as a test runner as it might delay other people’s work, increase costs or just annoy people with a bunch of failure emails — if that’s the case, run them locally!
How should you fill out a pull request?
Filling out a pull request means giving reviewers what they need before they start: a concise title built from strong keywords, so people can reference your PR later, and a description that explains how you would review the change yourself. Do not assign anyone until that is done.
Now think to yourself: how would I review this PR? Did I commit by files, modules or by context? Is it easier to review commit-by-commit or the diff as a whole? Use the description field to let your reviewers know! Don’t assign people yet!
What should you do before assigning people to your PR?
Before assigning anyone, take one more look at your diff and wait for the tests and other checks to pass. Remember that you are about to assign a task to someone: they will eventually stop what they are doing to devote some attention to your code.
It is a good idea to take another look at your diff — yes, again — because origin’s target branch might be different from what you’d expect, it’s better to catch these issues sooner than later. Also, wait for tests and other checks (if any) to pass.
You should also understand that GitHub has the concept of assignee and reviewer. To my understanding, a reviewer is anyone whose responsibility is to review your pull request.
On the other hand, assignees are responsible for merging the PR.
So, if you want to merge the PR yourself, only assign yourself! If you think it should be merged ASAP, you probably should also assign every reviewer, so they can feel free to hit the green button once everything is OK.
This practice is important to prevent premature/accidental merges. There are many scenarios where the PR code is ✅ but it still depends on some external factor. An example is a continuous delivery system that automatically deploys the master branch: sometimes you want to delay the deployment by a few hours, a day, etc. Having that rule in practice gives you peace of mind by knowing that you are the one controlling when the PR will be merged.
Something is broken ❌
Fix that broken piece! This is a good moment to use some advanced git skills like --amend, --fixup or rebase -i. Don’t feel comfortable using these commands? No problem! Do a regular commit fixing the issue and push it to origin.
All good now?
It’s time to assign people and wait for some feedback!
From now on, people are aware of your branch and might even have pulled it to their local machines, so it’s a bad idea to amend commits or fixups because it might cause undesired results.
What should you do when someone requests changes on your PR?
When someone requests changes, fix the issues and push the fixes, then tell your reviewers when you are done. This is great news, not a setback: someone devoted some attention to your code and kindly left you some comments about it. This might be a good moment to learn something new — or maybe it’s just a missing comma.
It’s time to get your hands dirty again! Fix the issues and preferably commit them individually or grouped by context — avoid doing general commits like fix all requested changes — someday you may need to traceback some code change and it might be missing a useful message.
Leaving the office but haven’t finished fixing it yet? You may push the commits to origin without worries! Your reviewers shouldn’t spend time looking into your PR again until you explicitly say so.
I fixed everything!
After pushing all fix commits to origin, give it a few minutes to run the tests again. Did they pass? Take another quick look at the PR. All good ✅?
Let your reviewers know! Add a comment to the PR mentioning the reviewer(s), a simple @username ping! will do it!
Good job, now relax
Wait for approval and ship it to the world.
Bonus tips ⛱
Amending a commit is quite simple: Let’s say you forgot to add some stuff in your latest commit, all you have to do is git add [files] then git commit --amend.
Fixup commits are a bit more complex: if you want to add some changes to a previous commit you have to git add [files] then git commit --fixup [commithash_to_amend]. That will generate a fixup! commit message referencing the commit you wanted to amend with. After that you just need to do an interactive rebase with git rebase -i --autosquash [basebranch] and it’s done!
Check out this awesome post my coworker Bernardo did about Code Reviews.
Did I miss something?
Any other practices you recommend? Think this is too much pragmatism? Leave a comment below, let’s discuss it!
