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. Initially I wanted it to serve as a database of typical PR clashes, allowing anyone to link to a relevant point and hopefully spend less time arguing. Is this even possible? Anyway, at the very least we have an article.
Note
Many developers were harmed reviewing pull requests which inspired this article, to which additions are expected as we invent new ways to confuse each other.
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?
Missing introduction¶
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
Copy-paste¶
Code with the same snippet pasted in a couple of places, when sent to review, isn’t a sign of respect to the time and attention of the reviewers.
This is noticeable in testing, most commonly in setup code when easily parametrisable snippets end up pasted into a number of tests. 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.
Uncalled for refactoring¶
Even if the refactoring change is in a separate commit.
Old and ugly code has one advantage over new code - typically at least one other person is familiar with it.
Refactoring inflates the changeset, introduces breakage, and requires the reviewers to do four jobs: learn the refactored code, determine which part is relevant to the change, review the refactoring, and review the change. Normally, only the relevant change would be reviewed. Unexpected refactoring isn’t free candy, it is also toil.
Exceptions exist - the code may shrink drastically, or the preexisting code could be too twisted to be reviewable, too fragile to change it safely, or not understood by anyone at all.
Implementation obscurity¶
The easiest solution to basic everyday obscurity are examples:
key, val = cryptic_string.split(';')[-2].split('=')
# e.g. cryptic_string = 'a;b;c=d;12'
key, val = cryptic_string.split(';')[-2].split('=')
You wrote some string-processing code? Give us sample strings in comments!
Moreover, it is a good practice to go through the code on the next day and add “why” comments for
anything non-obvious. In general, “what” should be obvious from the code, but “why” needs to be
explained. LLM-style # This is a comment type of comments are unlikely to be welcome.
Uncalled for experimentation¶
Present the simplest possible solution fit for the problem. Playing with code is great but not at the expense of your colleagues.
Most problems can be solved with structs, functions, tables (including associative ones, like dicts) and some purpose-made data types like queues. See Programming kept simple, part one.
Size¶
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.
Defensiveness¶
This problem exceeds the scope of this article, but avoiding the problems mentioned here will set the review process on a less adversarial path.
What is a PR?¶
A PR isn’t merely a branch we diff against its target and ask people to approve that. It is a method of communicating a changeset proposal.
If you excuse a virtue signal masking as a self-admission, I was guilty of some of the aforementioned points. I needed to develop a form of self-confidence to stop feeling the urge to overdo my work.
In the last couple of years, I began planning my work not only around the necessary change(s), but also around their reviewability.
You can write code for the computer. You can write code for yourself. You can write code for the so-called “next person who comes”. Or you can write code for the reviewer.
There is some room for disagreement here but I think the optimal path is to first write code for the reviewer and then, in a subsequent PR, change it for the future maintainer.