— Calvin Coolidge
Software development has always been a rich source of religious wars: emacs vs. vi, tabs vs. spaces, brace placement. Where do I stand regarding “pull requests” vs. “trunk-based” development? Even though it looks like the really hip developers opt for “trunk-based” these days, I’m clearly on the side of “pull requests”. But only if certain rules are followed.
Rule 1: Keep pull requests short
Don’t work on a feature branch for days. That’s against the idea of continuous integration. Not only do you risk integration problems, people will have a hard time reviewing your large changeset. Instead, split-up the pull request into several small(er) pull requests, for instance: preparatory refactoring, main implementation, clean-up, additional unit tests to improve code coverage. A pull request comprising 50 changed lines is easy to review, a pull request comprising 1000 changed lines, hard. What is easy to do gets done, what is hard to do usually gets delayed.
Rule 2: Constantly collect ideas on your way
Resist the temptation to cram more into your pull request than necessary. If you discover a badly named class or a way-too-long function, don’t clean it up right now. Make a note of it, add a TODO, or, even better, create a technical debt ticket. This keeps your pull requests short (see rule 1) and provides you and your teammates with work while waiting for feedback (rule 3). When implementing, review existing code and always be on the lookout for things to improve. Collect ideas for hard times like a squirrel collects nuts.
Rule 3: Be patient while waiting for feedback
Most developers, after having published their pull requests, only have one thing on their mind: integrating it asap. If they haven’t got any feedback or approval within two hours, they start hassling teammates, complain that they are blocked, do all sorts of whining. Such developers need to realize that while their changes mean all the world to them, they most likely don’t mean much to others. What developers should really have on their mind after publishing a pull request is this: a) are there any open pull requests from others that I could review? b) are there any technical debt tickets that I could tackle? If developers follow rule 1 and rule 2, there’s little chance that they ever run out of work.
Rule 4: Be kind and grateful when giving and receiving feedback
Nobody likes being criticized but every criticism should be viewed as an opportunity to learn for all involved parties. When you give criticism, be constructive and avoid cynical and arrogant remarks like “Only rookies would use a for loop here”. If you get harsh and unfair criticism, don’t take it personally and don’t add to the fight. Calm is contagious.
Rule 5: The author has the final judgment
I once was part of a team where people had a pull request battle over what’s the best way to initialize a string constant in C++. “Proof of efficiency” was shown by contestants in the form of disassembly for various platforms and they all showed different results. This ridiculous battle went on for days. Let’s not forget this: everybody is welcome to criticize the author’s code, but in the end, it’s the author’s decision what to accept and what not. The pull request doesn’t have to satisfy everyone and a software project is not a democracy. A good way to end a fight is this: “Thanks for your feedback, but I’ve decided to keep the code as it is”.
Rule 6: Advertise your pull request
If you want to make sure that nobody looks at your pull request, use a meaningless title full of spelling mistakes like “REwork for prformace”. If not, craft the title carefully to express it’s purpose and importance. I’m sure that the same pull request with this title will be reviewed much sooner: “Fix: Use a faster scanning algorithm to prevent nightly build timeouts”. The same goes for the description of the pull request. Give a crisp executive summary of the motivation and what you are trying to achieve. Don’t give long sermons to avoid TL;DR.
Rule 7: Review your own pull request
Before publishing your pull request, carefully review it yourself. It’s amazing what you can detect yourself, especially silly goof-ups and typos. Make sure that the pull requests is of highest quality, to the best of your knowledge. Don’t waste your reviewer’s precious time or they will soon start wasting yours by not looking at your pull requests anymore.
Rule 8: Publish only when you’re done
There’s nothing more annoying for a reviewer than having to constantly re-review because the author keeps on pushing changes. If you find out that your code needs rework, set your pull request to draft state. Publish it again once you’re done.
Rule 9: Proactively explain unusual changes
Experienced developers know that code shall be written such that it is self-explanatory and comments should be only used to document surprises. Still, certain changes to obvious, non-commented code might still be surprising to reviewers. In such cases, add an explanatory review comment yourself such that reviewers don’t have to ask you about it.
Rule 10: Spread the word
If teammates unknowingly violate these rules, educate them about these rules. If they don’t care or continue to deliberately violate these rules, let them wait next time they want you to review their pull request. Show, don’t tell.