Why your team will procrastinate reviewing your PR¶
or skim through it and slap an approval
This article attempts to summarise reoccurring problems in PRs, so that instead of having the same arguments all over, one can link to an appropriate paragraph and hopefully spend less time arguing.
Many developers were harmed reviewing pull requests which inspired this article, to which additions are expected as developers invent new ways to confuse other developers.
The points raised here remain valid in general, including greenfield development, however the magnitude of unnecessary suffering is significantly greater in legacy projects.
So, why?
Because we don’t know where to start¶
Most non-trivial PRs need a description which explains:
choices made and why a particular approach was taken
where to start reviewing the code and how to proceed
a section on alternative approaches that are plausible
Because you copy-pasted the same 150 lines of test code 4 times¶
As testing needs grow, test support code must be developed. You’re going to need reusable test setup, automated teardown, parametrisable tests, and functions for performing more elaborate assertions.
Because it combines refactoring with functionality changes¶
Even if the refactoring is in a separate commit.
We probably haven’t even expected refactoring. There are good chances that the refactored code isn’t instantly obvious, in which case, rather than reviewing changes on top of troublesome code which we were familiar with, we now have to work with refactored code which we don’t know at all.
Because the code is obscure for no reason¶
You wrote some string-processing code? Give us sample strings in comments! Ideally, go through the code on the next day and add “why” comments for anything non-obvious.
Because the implementation is a show-off¶
Or, the code is obscure for the reason of showing off.
Just because a tool is there, doesn’t mean you have to use it. Most problems can be solved with structs, functions, tables (including associative ones, like dicts) and some purpose-made data types like queues.
Because it’s just too big¶
Experienced developers are able to plan changes several PRs ahead. Even when this ideal appears unreachable, do not forget about it. It’s better to produce a chain of smaller PRs, or even parallel PRs with competing approaches, only one of which will win.
Because we are tired and you will be defensive¶
This problem exceeds the scope of this article, but avoiding the problems mentioned here will make us less tired and set the review process on a less adversarial path.
What a PR is?¶
Starting with a code-centric perspective, we’ll iterate, trying to make it more reviewer-friendly.
A pull request is a change to the source code which can be reviewed and approved.
A pull request is a change proposal which your team may approve or request changes to.
A pull request is a message to your team in which you propose an approach to solve a problem.
A pull request is a way to familiarise the team with a specific solution to a problem and discuss it before committing to it.
A pull request is a part of the development process in which a developer proposes a change to the source code upon a preexisting agreement as to what is expected to be done. This proposal is later iteratively reviewed and discussed with the team, which may result in an approval, a number of further change requests, removal of a subset of changes, splitting the changeset into 2 or more, or refusal to go with the proposal.