How to create awesome pull requests
That make everyone’s life easier (including your own)
Most code is added to the main branch via pull requests. Many eyes (and tests) find more bugs or simply confusing code that just your own, so most projects nowadays, either closed- or open source, incorporate code to the main branch only through PRs. That’s just as well, but there are two parts in a PR: the push part (that’s you) and the pull part (the other). I’ve been in both for quite some time, in both kinds of repos, so I thought I could share my experience here to help you save work, stress (in both parts) and bike shedding, as well as make a safer and enriching work environment for everyone. Here they go
A PR is a work in progress right up to the moment it’s pulled
But this is not the most important thing. The most important thing is that it’s a team work in progress. You happen to have picked that specific piece of work, but you need to work together with the rest of the team to get it accepted. You need to agree with whoever is reviewing it to eventually reach the best decision for the work team and the rest of the code base. So progress your work (and everyone else’s) together, up to it’s done to everyone’s satisfaction.
PR early, PR often
Since it’s assumed to be a work in progress, you’d better start that progress as soon as possible. The smaller, more atomic it is, the easier it will be for the rest of the team to review it, to check where it’s going, to progress to its termination. Do it often, too. You can have several PRs going on at the same time (while simultaneously reviewing other PRs). No big deal. Also, don’t try and land a 20 file, 4k lines of code PR only after it’s (according to you) perfect. That will only make it difficult (and uphill) for the rest of the team to review, not to mention accept it. Remember, every PR is (team)work in progress.
You can always mark the PR as a draft, just in case anyone really wants to merge it automatically. And then just revert draft state when you think it might be ready (it might not be, it’s going to be a work in progress no matter what, but still).
Use PRs title and subject to its full extent
Pull requests have a title and a body; sometimes they have to follow a template, often it’s a blank slate where you can write everything you want. Well, do write everything. Title should be related to what kind of problem it solves (more on this below), but the body should include some information that might be hidden in commits, and thus difficult to access, like why you’ve modified some specific file (that might seem unrelated to the main PR theme), the challenges you have met, whatever technical decisions you made and how you justified them. That might help whoever is reviewing to understand what’s going on, which is something the reviewer will be very grateful about.
Always follow the paper trail
Meaning you should only PR over an issue that it’s quite clear, and if possible, agreed upon by everyone. If you provide a technical solution to a specific ticket without discussing it first, you might run the risk of not considering the big picture, or simply plans that are in the roadmap and which you didn’t take into account when doing the PR. Document first, PR later; don’t do any PR over something some one said, even less so over something you understood about something someone said. Always work with a ticket or issue to support your work, as well as an architectural design record wherever possible. Also, use the tickets to generate meaningful branch (and PR subject) names; never call a branch something like you-know-that-thing. It should look more like fix/SECRETPROJECT/333/this-tiny-aspect-of-the-issue. GitHub will generate PR titles automatically from that name.
Limit the scope of the PR
Atomic, short, PRs over a few files, that address a single issue, are the best. Don’t bikeshed and be carried away when you find a bug in an unrelated part of the codebase: open a different PR (remember, PR early, PR often). That will also help your interaction with the reviewers: they might request something that falls outside the scope of the PR, for instance.
In most cases a PR was outright rejected, it was not because it wasn’t good code, it was because it tried to fix other stuff that was totally unrelated. All good and well, but all in good time too.
Don’t let your PR bitrot
PRs are forks from the main branch, and from beginning to the end of the life of a PR, the main branch will evolve, and sometimes create conflicts with the code in your PR. Check it out, fix it, and mark it ready for review. This is specially true in proprietary software, but it’s not a bad idea to keep tabs of all PRs you’ve opened in open source projects so that they don’t go stale. There was always a good reason (outside Hacktoberfest) for you to open that PR, keep it fresh so that it will help you when it’s eventually accepted.
Don’t pressure the rest of the team (or FLOSS developers) to accept your PR either
In the case of the latter, they might have other things to do, or simply not have enough time to review it. Generally, a FLOSS project will review, and accept, stuff quickly if
- It’s typo or something very minor or
- It addresses some outstanding issue they couldn’t address themselves.
Any other thing, it’s hit or miss. In any case, it must either pass tests or create new tests for any new functionality.
In the former, well, instead of pressuring the rest of the team into reviewing or, even worse, accepting your PR just create stuff that follows to the best of your effort what’s said above. In a nutshell, make it easy for everyone. It’s not going to be any easier if you get a hasty (maybe harried) review which, in case of doubt, is going to probably fail.
Remember it’s not you against the reviewer
It’s not your excellent work against a peevish and nitpicking reviewer (reviewers are gonna reviewer, and that includes nitpicking). It’s your work, as in the team work, creating value for customers or for the greater good in the case of a FLOSS project. A review might be wrong, and the person reviewing might be a jerk. Still, it’s probably the best to assume good faith by everyone, and suppose that what they want is the best for the team.
Remember that your code becomes legacy code as soon as it’s accepted. If you think that it’s you against the reviewer, you probably think that the code becomes SOP (somebody else’s problem) as soon as it’s accepted. It might be so in a FLOSS project where you’re an infrequent contributor (but if you are actually using it, it might become your problem down the road), but it’s definitely everyone’s problem if the PR is accepted and creates technical debt or simply does not do its job to full specs.
Work for the team
Remember that your work must be first for the customer, second for the team, third for your future self. Your future self will be incredibly grateful if a review becomes a learning experience from which a future, more fulfilling or better paying job can be obtained. Get your priorities right, and make your PRs a great (possibly educative) and always productive, experience for everyone involved.
