The software development workflow based on feature branches and pull requests is quite popular, with easy branching and merging supported by version control systems and pull requests existing as a feature in commonly-used source code repositories. Pull requests offer a way for the team to collaborate on the code asynchronously, not requiring the attention of more people at the same time, and in a way that works the same way for both co-located and distributed teams. But the way I often see pull requests being used feels to me more like inhibiting collaboration instead of promoting it. This post is about what I see as bad ways to use pull requests and what we developers can do to improve the situation.
My experience is from the corporate world, where there is a well-defined team working full-time, or mostly so, on the software and getting paid for it, and pull requests are used as a collaboration tool for improving the code and spreading knowledge about it. In the open-source world, where there is usually a core group of maintainers who accept contributions from outsiders, the social dynamic is different and not everything below is going to apply.
Terminology
The first problem is already in the terms, “pull request”, “reviewer”, “approval”. It sounds more like the pull request author petitioning an authority to accept their submission than a meeting between collaborating equals. I don’t know what would be the best terminology, but something like “merge proposal”, “collaborator”, “want to merge” seems better at emphasizing how the developers involved in the pull request do not have different roles but are all together collaborating to improve the software.
The whole thing gets even more problematic when considering the power relationships. In most development teams, there are more and less powerful people. They might be formally called “senior” and “junior”, they might come from different levels of knowledge of the code base, or they might just emerge from personalities. No matter what, unless the environment is psychologically safe, the less powerful can feel afraid of “reviewing” a more powerful developer’s code, whereas a more powerful developer can accidentally or on purpose end up bullying the less powerful ones by sounding strict and demanding.
Adversarial Communication
Pull request comments are not always the most fun place to spend time. This is perhaps an outgrowth of the previous point, on how pull requests are viewed, but the commentary can get surprisingly hostile, considering that everyone is supposed to be in the same team working towards the same goals. Sure, the submitter of the pull request may have used an interface in the wrong way or introduced a bug, but everyone makes mistakes, and the whole purpose of doing code reviews is to catch those mistakes before they make it into shipping code. There is no reason for abusive language or inferring general traits from individual mistakes.
One case especially where I see an adversarial mindset in code reviews is when the pull request “touches the reviewer’s code”. Some developers get very attached to their exact vision of how the code they wrote should look like and be used, and will get defensive if someone else doesn’t see things exactly their way, and this can lead to very hostile comments. I am a big fan of collective code ownership, where the whole team is collectively responsible for all the code, with no individual owners of any piece. Naturally, modules and such often have people who know them better, and getting their input is valuable to avoid making mistakes, but that doesn’t mean that these people get actual ownership of these parts.
With an environment like this, it is no surprise that when given the ability to select the reviewers, developers often go for the ones that give easy approvals without the usual harsh comments, which again reduces the amount of real collaboration that is going on.
Gatekeeping
The pull request systems I’m familiar with support enforcement of the pull request process. Typically, this is done by blocking direct pushes to the main development branch, requiring pull requests instead. Then, the pull requests are configured to require the approval of specific people, or a certain number of people from a specific group, before allowing merges. The reason given for this is typically to ensure that all code is seen by more than just one person, and that the people most familiar with the code base are among those.
In my experience, it is rare that a team would decide to turn these gatekeeping measurements on by themselves. A team that sees the value of collaboratively developing code has an agreement on how the collaboration happens, and often this might mean an agreement that code is submitted through pull requests. But turning on the enforcement part is more often the purview of management. And even if some process is beneficial, having it imposed on the team by an outside agency is detrimental to team morale, and in this case, it can lead to team members not caring about the code review anymore, just adding their approval tick marks semi-automatically to make sure all hurdles are cleared.
Improving My Behavior
I have been guilty of at least some of the sins above, but I’m trying to improve. The most important issues I observe in my behavior are being too dogmatic and judgemental, especially towards developers less experienced than me, and it is only quite recently that I’ve began seeing my communication style as problematic. These are some of the things I’m trying to do, and I feel like they have a positive effect in the team and make me also a person that others are not afraid to approach with problems.
If I see something that looks wrong to me (not “wrong”, I’m trying to let go of an absolutist view of code), I don’t go demanding changes. Usually, there is a reason why the other developer did what they did, so instead I ask them to get their reasoning. At this point it can be counterproductive to even mention why I think this looks wrong, since the other developer may feel that I’m demanding them to change the code despite how the message is phrased. Even the “actually wrong” things are often caused by missing context of the code, which points more to poor documentation of interconnections than to the abilities of the other developer.
When I have more experience with the code base than the other developer, I often know functionality it has that can be useful. So if they have written some code that we have already made into its own function, I again don’t adopt a demanding tone that they change it, but try to be more in a helpful mode of pointing out something they likely didn’t know about.
I try to also give praise when I see something nice. If the other developer has figured out something tricky, refactored some code neatly, or just done something in a clean way, I really should leave a good comment about it. Otherwise, the pull request commentary can become an exercise in negativity where only problems are commented on instead of a collaboration aimed at improving the software and the code.
I have fought against gatekeeping. If I have been managing a repository, I have not enabled any gatekeeping features, preferring to rely on the team’s judgement on when a pull request is appropriate and who should be checking it, and to allow developers to push directly to branches if they are confident enough. This is the normal approach of making people trustworthy by just implicitly trusting them. Unfortunately, I often get overruled eventually on this. I have tried to ask why enforcement of these gatekeeping policies is necessary, but no one ever provides an answer to that question, I am only ever told why it is useful to have code reviews. To some organizations, trusting employees seems like a completely foreign concept.
Experiences
I still remember one of the first times when I gave a positive comment on a piece of code in a pull request review. I had perhaps not been as clear as I could have been, since the other developer, with the apparent understanding that comments mean problems, changed the code that I thought I had praised. This did expose to me the mindset that I, too, had been guilty of regarding pull requests. That the reason for making them is to find problems in the code, not to collaborate with the rest of the team.
My increased focus on asking questions instead of making demands also hasn’t worked as well as it could. I would genuinely want to get answers to my questions, and if needed, to use them as a springboard to discussing the changes. But also here, a comment is too often seen as a demand for change, so instead of answers to my questions or discussion on the design decisions, I just get a laconic comment “Fixed” and something got changed even if it wasn’t necessary.
I think that to make better progress towards collaborative instead of adversarial code reviews requires setting up team practices right from the start. Make sure that everyone understands the concept of collective code ownership, that everyone is on board with leaving their egos behind when approaching a code review (unlike often advised, this applies to the reviewer just as much as to the person whose code is being reviewed). Read comments on pull requests even when you’re not involved, and try to point out toxic and adversarial communication (sometimes in public, sometimes privately to the person, it depends on the situation). If bad behavior takes its root in a team, it can easily become the norm and be very difficult to root out, so it is important for the whole team to ensure it doesn’t become “how things are done here”.
Latest comments (0)