Programming

PR Reviews: not the messiah, not a naughty boy

On Tech Twitter there seems to be a lot of chat recently about how the Github Pull Request (PR) review process is “broken” or underserves internal teams (an example) that have strong collaboration practices such as pairing or mob coding.

Another objection raised is the idea of internal gatekeeping within development groups, I’m not sure I fully follow the argument but I think it runs along the lines that the PR review process allows powerful, influential members of the group to enforce their views over the others.

This is definitely a problem but frankly writing AST tools linked to the “merge to master” checks is probably a more controlling tool than spending your time policing PRs.

With open source projects often all the action happens at the pull request because the contributors often have no other interaction. Proponents are right to point out that if this works for open source projects do you really need to put effort into other practices upstream of the PR? Opponents are also right to point out that adopting the work practice of an entirely different context into your salaried, work context is crazy. They are both right, individual organisations need to make deliberate decisions using the challenges of both side to the way they are working.

I’m not sure how much of this is a fightback by pairing advocates (that’s how I interpret this thread). There is a general feeling that pairing as a practice has declined.

In my work the practice is optional. As a manager I’ve always known that in terms of output delivery (which before people object, may be important particularly if you’re dealing with runway to meet salary) pairing is best when you’re increasing the average rather than facilitating the experienced.

I think even with pairing you’d want to do a code review step. Pairs are maybe better at avoid getting into weird approaches to solutions than an individual but they aren’t magical and if you are worried about dominant views and personalities pairing definitely doesn’t help solve that.

So I’d like to stick up for a few virtues of the Pull Request Review without making the argument that Pull Requests are all you need in your delivery process.

As an administrator of policy who often gets asked about Software Development LifeCycles (SDLC) as a required part of good software governance. It is handy to have a well documented, automated review process before code goes into production. It ticks a box at minimum disruption and there isn’t an alternative world where you’re just streaming changes into production on the basis of automated testing and production observability anyway.

As a maintainer of a codebase I rely on PRs a lot as documentation. Typically in support you handle issues on much more software than you have personally created. Pairing or mob programming isn’t going to work in terms of allowing me to support a wide ranging codebase. Instead it’s going to create demand for Level 3 support.

Well-structured PRs often allow me to understand how errors or behaviour in production relate to changes in code and record the intent of the people making the change. It makes it easier to see situations that were unanticipated or people’s conception of the software in use and how that varies from actual use.

PR review is also a chance for people outside the core developers to see what is happening and learn and contribute outside of their day to day work. Many eyes is not perfect but it is a real thing and people who praise teaching or mentoring as a way to improve technique and knowledge should be able to see that answering review questions (in a suitable form of genuine inquiry) is part of the same idea.

PRs also form a handy trigger for automated review and processes. Simple things like spelling checks allow a wider range of people to contribute to codebases fearlessly. Sure you don’t need a PR to use these tools but in my experience they seem more effective with the use of a stopping point for consideration and review of the work done to date.

Like a lot of things in the fashion-orientated, pendulum swinging world of software development good things are abandoned when no longer novel and are exaggerated to the point that they become harmful. It’s not a world known for thoughtful reflection and consideration. But saying that pull request reviews undermine trust and cohesion in teams or a formulaic practice without underlying benefit seems unhelpfully controversial and doctrinal.

Standard

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out /  Change )

Google photo

You are commenting using your Google account. Log Out /  Change )

Twitter picture

You are commenting using your Twitter account. Log Out /  Change )

Facebook photo

You are commenting using your Facebook account. Log Out /  Change )

Connecting to %s