Pull requests (PR) are part of the developer workflow within Github. You may hear this term as merge requests in Gitlab or other tools. The workflow for developers on my project is managed within Github. This is where user stories are listed for the sprint as issues and developers assign themselves the issues during the sprint.
work·flow
noun
the sequence of industrial, administrative, or other processes through
which a piece of work passes from initiation to completion.
Developers work based off the requirements within the issue and then make a PR when the work is completed. While the developer waits for a code review on that PR they will typically assign themselves another issue to go work. Now the developer has two issues - one in code review and another in-progress. Let's say the developer finishes the 2nd issue and makes another PR, they now have 2 PRs in code review.
This is where the 2 PR rule comes in to play. Once the developer reaches this point, they should not pick another task until one of their current PRs is merged into the baseline. This rule is mainly to encourage the developers and the team to review code, provide feedback and get the code merged. While the developer isn't supposed to pick up another task that doesn't mean they shouldn't be working. During this time the developer should ask other developers for a code review. The code review will typically result in comments and requested changes or get approval to merge the code. If there are requested changes the developer is able to immediately begin addressing those comments and update their PR using this rule. If their code is merged then the developer is free to assign themselves another task to work on.
There will be times where no other developers are available to do a code review because they are focused on their own task, bug, or fire. In those cases the developer waiting for code reviews on their 2 PRs should seek out another developer who may need help and pair-program. They say two minds are better than one and pair-programming can help a struggling develop get over a technical hump with another set of eyes. By pair-programming the idea is that the work will result in better quality code and merged in to the baseline faster.
I have seen problems arise when a rule like this wasn't in place. Mainly less code is delivered by the developer because their is no focus to get code merged or address comments in a timely fashion. To illustrate, take for example a developer who has 4 PRs and assigned themselves another task to work. They now have 5 tasks in a state of code review or work-in-progress. As code reviews for the 4 PRs happen and comments are provided, the developer is focused on their 5th task and works to get a PR up for that one. When they eventually get around to addressing comments they may now also have file conflicts they need to resolve that resulted from the time it took to address those comments. Once they address those comments it may take some time to get another code review on those PRs. So this developer now has 5 PRs and nothing has been delivered to the baseline. If this process continues, then the developer begins work on a new task which escalates the problem. Eventually the developer is the bottleneck to get many tasks completed.
I summarized the pro and cons from my experience of using this approach.
Pros:
- Focus on delivering code
- Encourage developers to do code reviews (everybody participates)
- Help other developers who may be struggling through pair-programming
Cons:
- Potentially delay other work from starting
- Can be frustrating for developers waiting on code reviews
I'm interested in hearing your thoughts!
Top comments (6)
This is a very good rule! I think I'll suggest we start to use it in my team on next retrospective meeting. Even though my team is small now, we do tend to have problems with forgotten pull requests hanging around. And you are right that getting back to work you did 5 pull requests ago is quite expensive in terms of context switching.
There are actually other things that can help. In my previous company we were often reminded that reviewing PRs is probably most important task for a developer, because it (un)blocks other people. It was a good practice to always look if there are some PRs to review before picking up any new task.
I didn't even think of context switching while reading this post - thanks for bringing it up. That's a great point. Even if each pull request only took you a day to work on, you might have to context switch to a week ago at the very least.
Also, if there are 5 pull requests hanging around, it's possible something else is broken as well. Maybe those pull requests weren't the most important things to be working on? Maybe a QA team is backed up with other testing? Maybe nobody is focused on releasing? There could be a bunch of other factors in addition to the development team.
Great point Paweł! I didn't mention context switching but you are right it is a hidden cost that is being paid.
You could try to experiment to see if 3 PRs per developer is the right number for a smaller team.
I really like the concept! As you wrote, it puts a focus on actually delivering value to the product. As long as things are unmerged (aka "I am done, it just needs to be reviewed") they have nearly as little value as if they were not started at all. Changing the mindset to see how the team can get to merge something ist the right thing to do. As often, this rule makes an underlying problem visible, but it does not cause the problem.
As with most rules, I wouldn't be dogmatic about it though. If there is a good reason to break it, people should do it, but they should also be able to explain why. (And "I needed to do another task" is not the right answer here ;) )
Thanks Philip,
My last team used this rule and even though it caused some frustration for developers it really put the focus on delivering code. I definitely agree with you there are some cases to break it. Some obvious times I have seen it broken were much of the team was out of the office so no one to do code reviews. Their 2 PRs were so small (a few lines or less) that it made sense at the time for them to take on another task while they waited for code reviews.
I think this is a good solution to a specific problem. If people routinely ignore open pull requests that need reviewing and start taking a multiple-day lag time for granted, making or encouraging them try to get other developers in line to review their stuff makes sure they're walking the proverbial mile in everyone else's shoes.
If you have people making many smaller pull requests rapidly -- in different areas of a large codebase, or just in general to keep a legible change history -- and some of them occasionally wind up waiting a little longer, it's a statistical blip that doesn't particularly need addressing.