Pull Request based development (sucks)

  • Published: 2017-03-18
  • Modified: 2017-03-18 22:40
  • By: Mishoo
  • Tags:
  • Comments: 8 (add)
Mar
18
2017

Pull Request based development (sucks)

Does this workflow sound familiar? The “master” branch is considered sacred, and it's therefore locked so nobody can push directly to it. Before you even start coding, there has to be a task opened in JIRA, and a branch also created from JIRA and linked to that task, and you then start pushing commits to that branch. And when you're done, you submit a Pull Request via Stash, and wait for at least two colleagues to review and approve your patch, and only then can it be merged to master.

As a developer who has been around for almost two decades, I'm going to argue why this workflow sucks. You tell me to run, and then you tie my legs.

My development workflow

In my world, fixing a bug usually goes like this:

Sometimes it takes less than a minute from the moment the problem got mentioned to the moment I push the fix. The fix goes to the master branch so everybody gets it immediately. If there are problems with my fix, they will be noticed quickly.

The “desired” workflow

I partially ignore this, but here is what we “should” do, according to company policy:

I highlighted in bold above the steps that are actually required to fix the problem. The rest is bureaucracy. As a developer, I hate bureaucracy.

The proponents of this workflow claim the following advantages, to which I respond:

Any other advantages that I don't know of?

Let me pause a minute to tell you that I'm not against bug trackers. But perhaps not all issues should be logged. You don't file a ticket which says “the headline font size should be increased by 0.5px”, you just go and do it. You do file tickets for more profound issues or feature requests, though. Unfortunately, the bureaucracy imposed by JIRA makes me dislike it, and I avoid it as much as I can.

Nah, just JIRA. A great bug tracker, for instance, is the one at Github.

The fallacy of “code reviews”

So, you probably have a few tickets in your backlog, and are working on something, and I suddenly ask you to review my pull request. First, let's note that I'm interrupting you from whatever you are doing, which is bad. Anyway, there are two cases here:

  1. My branch contains a trivial fix, like adding a bit of CSS, or changing a couple of lines of JS. You quickly look at it in Stash, you assume I actually tested it, and you hit “Approve”. The value of your review is exactly zero.

  2. I've been working on my branch for a week, implementing a new component. The patch is +1000-200 lines of JS and +50-30 lines of CSS. You gaze at its greatness in Stash, you assume I actually tested it, and you hit “Approve”. The value of your review is exactly zero.

Yes, folks, this is what really happens! If you're wondering why the second case goes like this, here's why: it took me a week to work on that code. In order to understand it, you have to basically rewrite it in your mind. You don't have the mental context that I've built over a week, because you never thought about the problem that my code is trying to solve. It's simply not possible to provide a meaningful review in a short period of time, that's why you click “Approve” without thinking. You have your own issues to work on, and my interruption already did some damage by de-focusing you. Reading code is hard. Nobody really does it.

At best, you can provide stylistic objections, like “hey why did you indent code like that?”, to which I respond “I don't indent code; that's editor's business.”

Required code reviews are not just useless, they actually incur some costs:

“So what is the solution?”, you ask. “How do we keep the master branch stable?” The answer is: you don't. As has been seen time after time, things happen exactly as I described above and sometimes a bug finds its way into the master branch, and you have to deal with it. By not allowing the fix to be pushed quickly, you're making it worse! The master branch enables your developers to collaborate, to share code. Don't take that away from them; allow direct push to master! What you should do instead of locking master, is to have a “stable” branch, and periodically reset it to master when master was tested extensively and has passed the tests. And that's when you do a new release.

But wait, speaking of mandatory code reviews, did you think about this? So you don't trust your own developers, and require attention of at least 3 people for a commit to get in. But, do you use any frameworks or libraries? Do you use Nginx? Do you use Linux? You trust tens of thousands of random developers all over the Internet; your business runs on their work. Your node_modules has 247.5MB and I'm telling you, most of it is crap; yet you trust it to run the infrastructure that you depend on daily. Did you review any of that code?

Please, people, kill the pull request! Open the development branch!

Footnotes
1. Did you notice that testing comes after merging? Doesn't this kinda defeat the goal of having the master branch stable at all times?
8 comments. This is HOT!

Add your comment

# Yevgen
2017-03-19 15:27
I think the gains of more formal PR review process (a la bureaucracy ) are the following: - the bigger team -> more valuable the formal process will be. - PR based approach allows running tests/lint in CI before it will be merged into master. How often we face a problem "works on my machine"? - not necessarily all PR will be review by all engineers in the team. In most cases will be 'aria experts' that could give valuable input into PR changes - PR allows better knowledge sharing across the team, especially for newcomers.
# anonymous
2017-03-23 16:18
Generally I agree that receiving 500+ patch and staring at it for some time is pretty much useless. However, code review + "white box" testing (when code reviewers actually run _their_ own test cases against your new changes in order to break them) really changes the quality upward by far. Not all code is that critical obviously.
# Brandon
2017-03-23 16:31
Reading this I noticed some issues, and its with how the bureaucracy around you functions. First off, tests should come first not last, and a piece of functionality that is properly tested should never break master. Not to mention that a full build of your branch(which is off of master) should work properly with all tests passing. Therefore your master branch stays the same. As far as your comment on "noone reads code" I would challenge that with everyone that is a good developer reads code regularly. In fact code is read far more than it is written. I'd say a solid 2 hours of each day goes to reading code. This is a normal part of being a good developer. lastly, if your argument is that code reviews are pointless bureaucracy you shouldn't contradict yourself by referring to open source projects that have heavy amounts of bureaucracy and require lots of code reviews before any code makes it into the code base. The truth is, that the bureaucracy exists because people are error prone. we make mistakes all the time, but by making a team come to consensus on a change then breaking changes are less likely to ever happen. My suggestion is, take a step back, have a moment of reflection on why you are wrong and work to be better. Your team may just benefit from it.
# Mishoo
2017-03-23 17:59
Right, I should have mentioned that my rant does not apply to open-source. Indeed, you can't open the repository for anyone to push directly; for open-source (or, yeah, large distributed teams) PR may be the way to go. I'm talking about a closed product and a relatively small team.
# sbarzowski
2017-03-24 14:12
There's more to code review than just looking for bugs, that's just a side-effect. It's mostly about code quality and going in the right direction with the architecture. There are things that other developers might have a better chance to catch, for example: * Mixing abstraction layers * Overcomplicated code * Bad names * Obsolete comments * Missing tests * Backwards compatibility issues
# TRB
2017-03-24 18:02
One major difference as I see it is that OSS projects are not a team of hired individuals whom you should trust but literally anyone on the Internet - whom you should definitely not implicitly trust. Also I think the scale and scope of the code base and change matters. Changing the color of text of moving an image around a bit is not the same thing as refactoring a series of conditional operations.
# TRB
2017-03-24 18:05
I would assert that PRs are precisely the wrong place to be doing "architecture" work. I'd even go so far as to say that the only architecture related work in a PR is validating that what was written implements what was designed. Architecture work is done in advance, agreed upon, and then the PR implements it (or a subset of it). If you're deign system design review/changes in PRs you are not doing system design or architecture, you're doing organic growth of a system.
# Dali
2017-04-13 14:10
Perhaps it is just companies trying to imitate the bureaucracy of open-source pull-requests, badly. Hired individuals should be people you trust to push directly to master. I have a suggestion: tag releases instead of branching.