Authoring Pull Requests

#1. Keep em small

It is an art form to keep Pull Requests (PRs) small. It’s very tempting to rewrite, refactor, boy scout and reformat the code as you develop but in general, the best developers resist the temptation to change everything at once. They stay on target and get the job done with minimal code changes. Some even compete to have the highest ‘lines deleted’ to ‘lines added’ ratio. If boy scouting and refactoring is necessary, do it separately. Don’t invent excuses for why it’s easier just to shove it all into a single PR — that’s being lazy. Instead, invent ways to make smaller PRs happen — that’s being creative.

#2. Perform a Self-Review

Create a draft PR and do a full review yourself — complete with comments and tasks. After finishing a piece of code, it's very tempting to just dump your changes in a PR and let other people find the mistakes, particularly if it is a larger change that took a few days. Don’t be lazy — be disciplined, your work is not over yet.

You can also use this “self-review” to point things out to your reviewers — “I’m not sure about this name? Can you think of a better one?”, “Should this really be nullable?? What do you think?” — often while writing such questions, you find that actually, you can answer them yourself and the self-reflection becomes built into your everyday coding thought process. In other words, this process of self-review makes you a better developer.

#3. Go back and remove noise

Often while self reviewing, you come across a file with only white space changes, formatting changes, optimized imports or some text change that has no impact related to the intention of the PR. Make the effort, go back to your branch and revert these files to how they were before, it doesn’t matter that you have slightly improved them — functionally they are the same, and the extra file in the list of ‘changed files’ listed in the PR provokes groans from the reviewer and lowers their motivation to do a proper review — especially on larger PRs.

If formatting is important, create a separate PR, introduce a linter, make it a part of your CI, and format the entire app in one big nasty PR. But keep noise away from the important changes — respect your reviewers’ time and energy.

Furthermore, such noise pollutes the git blame history, making it harder for people to browse history to discover the intentions behind certain changes to a file.

#4. Create a meaningful title

Often the title is generated from the branch name or related ticket… the only rule here is to follow some kind of convention and to make the title short and meaningful, the thought process is similar to naming branches. Be consistent.

Pull Requests are the new Documentation, making Pull Request history easy to browse makes searching for past decisions and thought processes much easier.

#5. Write a helpful description

Again, treat the PR as documentation — documentation that you would actually read because it is useful. Be as thorough as possible, but be concise; try to preempt discussions by being as transparent as possible about your intentions and decision-making process.

A useful way to promote inspiration is to establish a PR Template. The contents of the template should be agreed upon and developed/adjusted with your team over time, but here are some ideas for starters:

Overview of changes. Things to address here are often what is not in the PR — alternatives that you have assessed and discarded. This preempts potential discussions with reviewers who may suggest things that you have already tried.

Questions/Notes for reviewers. Any parts of the code that you need some specific advice on? Any sections of the code that can be safely ignored? Maybe you refactored the name of something and this touched a lot of files, but had no functional impact. Notifying reviewers that they can safely ignore a chunk of the PR is going to be well received.

How to test/demonstrate. This is handy for QA — user/pwd combinations that you might want to use in the staging environment, configuration/preparation/setup instructions… anything that will help the reviewer test the changes.

Attachments: Screenshots, Videos. A picture is worth a thousand words, a video is worth a million. There’s no substitute for seeing the changes in action — creating proof of completion is also a handy artifact to have when Demo day comes and you have forgotten to prepare!

#6. Acknowledge each and every comment

With any remote and asynchronous communication, it’s important to communicate that you have at least seen a comment that was meant for you. It might be with a simple emoji reaction, fine! Never ignore a comment, no matter how small, especially in a new team. Once you get a feel and build rapport with your team, ok, then you can let a few slide because you have an understanding. But at the beginning, bring out your table manners, speak the Queen’s English and be on your best behaviour.

#7. Don't merge until everyone approves

Speaking of manners, it’s good manners to wait until everyone who has made a suggestion has had a chance to hear your response and evaluate it. If you have been waiting for a relatively long time, gently ping them out of band (email, instant message, tap on shoulder) and let them know you’re waiting.

IMO if you are on a team of 3+ people, it is not necessary for everyone to review every PR. Work out a system for determining who should review each PR — perhaps certain people are responsible for certain modules, perhaps when you are new the architect/senior dev will review all of your PRs until you graduate, maybe you use a round-robin. There are many ways of dividing up the work, be creative.

Reviewing

Some of the advice below also applies to the author while replying to comments.

#8. Check out the code

Always have two clones of a project on your computer at any one time. One for normal work, one for reviewing PRs. This way, you can put your current task on pause with zero overhead.

Once you have checked out the branch, press build and leave the build running while you switch back to the browser.

#9. Read the title and description

If someone has put the effort in to write a guide to their PR, the least you can do is take the time to read. So while the project is building on your other project clone, read the linked ticket, read the PR title and description — prime your expectations for what you are about to review.

#10. Validate your suggestions locally

When you find something that could be improved, try to make the change in your local clone. When new to a project, this is especially important. There’s nothing more embarrassing suggesting a code change that is not possible and doesn’t even compile. But more than that, you get a better feel for the code when it is in your IDE and as you make changes, you might be either inspired to make a larger refactor or realise that your suggestion would have been fruitless. Both are equally important and valuable outcomes, on top of simply validating your suggestion.

Once you have verified that what you are about to suggest is at least possible, don’t waste the effort that you have gone to, copy the code changes and put them into a code block in the PR comment so that the author can copy it straight into their branch.

#11. Convert large suggestions to a Pull Request

If you found that your suggestion became quite large, don’t waste your effort: branch off their branch and create a PR to merge into the original PR. This new PR doesn’t need all the bells and whistles of a regular PR, because it’s just a glorified comment, but it provides a separate arena for a discussion over a larger change and this change can be played with by the original author and finally merged into the main PR if all goes well.

#12. Resist the urge to comment

The hardest thing about making comments is resisting the urge to make comments. The key to restraint is to be as hard on yourself as you are about to be on others. If you ever wonder how effective you are as a reviewer, go back through all of your comments and measure how many of your comments result in your suggestion being well received and implemented with minimal back and forth.

Here are some handy hacks to improve your comments:

#13. If you don’t have a concrete alternative, don’t make a comment.

If you simply don’t like something, come up with something better and make your case, otherwise, zip it.

❌ “Hmmm yeah I just don’t like what you’ve done here.”

❌ “I don’t really like this approach.”

✅ 🦗🦗🦗🦗🦗

#14. Be confident, not lazy.

Don’t use words like “maybe”, “I don’t know”, “What if”, “I’m not sure”… anything that implies doubt. If you’re not sure, self-reflect — why aren’t you sure? Do some experiments or research … come back confident.

❌ “I don’t know, but if you make this change it might be better. Check it and see what you think.”

Lazy: you don’t know? Don’t comment until you do know.

Confident: if you do know, don’t say that you don’t — be confident, state your suggestion and be clear and outline why it is better without referring to your own opinion (see next section on Style).

✅ “If you refactor this into an inline function like so [code block] the code becomes smaller and more idiomatic. It also avoids code duplication. However, it is not as easy to mock while testing.”

Notice that last piece about it being harder to test? There’s nothing wrong with outlining and pre-empting the negative aspects of your suggestions. It shows that you are thinking objectively and rationally — no solution is ever perfect.

#15. Mimic style conventions before changing them

People are strangely attached to style and become very opinionated very quickly. When joining a new team, existing members will often defend the project’s status quo, and new team members are often vocal about how they “did it better/differently in their previous project”.

Rise above it.

Adapting to an existing style will win you brownie points with your new team and leave them more open to your suggestions on more important things: choice of SDK, architectural decisions and patterns and practices. After you have mastered their style without complaint, you will find them a more receptive audience when you make hints towards style modernisation.

#16. Nitpicking is a good sign

If your peers review your PR and you get nothing but style correction comments, it can seem like nitpicking which can be frustrating.

But look at it like this: the reviewers are struggling to find any real problems with your code.

Some good responses to nitpicking is to politely highlight how inefficient it is to nitpick when there are IDE tools that will handle 90% of all style decision automatically. Usually it is a symptom of laziness, lack of motivation or poor leadership when automated style checks don’t exist — so create them yourself!

“Oops, I wasn’t aware of that style rule 😬 sorry. Hey is there a style guide you could point out to me in confluence/wiki/readme?”

“Ok, what about if I create a separate PR with all these rules you’ve pointed out? I will dump all the comments I receive into this document so that our style is documented and can remain consistent.”

“Ooops, sorry this is the 40th time I have missed this. Hey, would anyone mind if I created a linting rule for this? I could add it to the IDE so that it is picked up while coding, or perhaps to a git hook so that it is picked up before pushing or even to a CI bot that would scan the PR make a comment directly.”

#17. Take long threads to a meeting room

At some point in your career, you will get into a heated argument over a comment in a PR. The key is to stop the conversation, cool off, then write the person an email that goes like this:

Hey Dickhead,

Sorry about this comment war we’re having in the PR, things are going a bit sideways and I think it’s time we either got together on a call/meeting room, maybe with our immediate superior or some other team members and bash it out the old fashioned way — in person. Take your time responding, there’s no rush. We can get back into it tomorrow after sleeping on it.

Yours sincerely,

Asshole.

The key is to break out of the childish Twitter-induced cycle of abuse and get face time. People are never as brutal or rude in person as they are online. Just make sure you come to that meeting cool as ice with your points and counter points well thought out, but mostly, stay objective. Go into the meeting with the goal of finding the best solution, not to get your own way.

#18. Keep your tone of voice even

To create a Pull Request is, by definition, to invite criticism. So first and foremost: be critical. Poke, prod and challenge — but do so professionally, not personally.

Use emojis for fun moments,

✅ “Yay! You used Flow 🤩”

Don’t use emojis in a transparent attempt to cover up your complete lack of tact

❌ “We use spaces in this project. Change it. 🥳 🚀”

Avoid “you”…

❌ “you forgot a variable here”

… try “we”

✅ “We are missing a variable here”

Or passive tense instead

✅ “There is a variable missing here: [code block]”

Be mindful of how you interpret comments — chances are someone didn’t mean to be an asshole, they just weren’t thinking, or were in a rush, or English is their second language, or they got distracted. Assume that people have good intentions. Even if you are 100% sure they are being evil, there is a 10% chance you are wrong. Try to find a way to internalise their comments and avoid escalation. Watch Russell Brand when he came to America for some inspiration on how to do this.

#19. No emergency Pull Requests

Pull Requests became popular for two reasons:

1. Asynchronous communication. Developers can review and respond at any time, allowing them to continue development without interruption and review PRs when they come to a natural pause in their flow.
2. Quality Assurance. Reviewing and testing code before it hits the target branch ensures that the target branch stays clean. Teammates are excellent at spotting potential problems in code they work with day-to-day.

If you absolutely have to merge a branch in the next 10 minutes (and there are plenty of situations where this is a totally sane thing to do), don’t send out a message asking people to review it right now, just merge it. Don’t bother with a PR just for the sake of The Process — you’ve just interrupted everyone and asked them to perform a mediocre review — negating the two main benefits of a PR. If you have a branch that must be merged in a short time frame, interrupt one person and do some pair programming or just merge it and suffer the quality hit.