How does your team handle pull requests? Do you have any kind of practices around them? Do your peers usually stop whatever they are doing and prioritize reviewing open pull request or rather prefer avoiding the context-switching until they're done too?
I have some thoughts around it but I'd rather hear from the community first.
Latest comments (12)
Depends on the personality. I have an example from my team - I need to explicitly order him to review a PR and there is no other way. But in most cases, just a short talk about the pros of mandatory reviews for all incoming PRs should work.
If you're using an agile development system, prioritize assigning points to developer stories to look at pull requests.
I'm not sure I got your point: do you allocate a fixed amount of your development life-cycle to pull request review?
I'd say that's what he means, yeah
Funnily enough I recently wrote a pull request guide for my team about the best practices for a good PR, both for the creator and the reviewers. Though that focus was on increasing the quality of the PRs (we'd started having too much rubber-stamp behavior with PRs) than the issue with people not reviewing that you are experiencing.
I've had prior experience with the 'people not reviewing their PRs' problem, and there are different root causes. Like @samuraiseoul said you should ask the team to find out what they say is the reason before you try to fix it.
The most common cause is the priority problem, where people prioritize working on their stuff rather than reviewing the work of others. This is actually a respect problem, because fundamentally these people expect that others will review their work so it can get in but don't spend to time to do the same for others. With high time pressure people don't always see it that way but it is the truth. The approach here is to get people to understand this and that to ensure that the team is successful everyone needs to take time to review other peoples code so that everyone's code can get in. In general you shouldn't see much push back with this because most people want the team to be successful. There are always exceptions of course, like when a high priority issue has a hard deadline, but if you have people who think that their work is always more important than others then you have a different problem - a toxic team member that you need to get rid of.
Another problem is when particular people get swamped with PRs. Most of the time this is because they are considered the subject-matter-expert so everyone who touches that area adds them. You can also see the case where you get individuals that are just really good at PRs so people like adding them. What you need to do here is to identify and get everyone to acknowledge the issue before you can fix it. Then set guidelines like only including SME's if it's an important and/or risky PR. How do you know that? Ask them! Something I love is when people have a quick chat about a change and ask "do you want to review this?". SME's are in a great position to evaluate the risk factor and say "yes, I need to look at that" or "no, that sounds fine and ${DevX} can handle that review".
The last problem I've seen around this is when a bunch of people add too many people on each PR. Two heads are better than one, that's the whole point of PRs, but that doesn't mean you need five. I think you tend to see this more in junior people, as they're not sure who to add so they add everyone. And occasionally it can be good, like when trying to get feedback about a new approach to something. I personally did this recently with some React hooks code because a) we didn't have a well established pattern on how to use hooks yet, and b) I'm a React noob so I wanted to get some broad feedback. But that should be the exception, not the norm.
Really valuable feedback, thank you very much.
For each source repo, there is one owner who is in charge of approval pull requests.
I think feeling of ownership is super important.
I find that most developers I meet who are actually good coders love the idea of pull requests!
Unfortunately, though seniors think they are too busy, and juniors think they don't know enough nor how to contribute anything meaningful. :( Then past that some PRs are just too massive for anyone to do things with.
Luckily there are solutions for these issues!
For the Juniors, do a few pair code reviews. Have a senior go through and show them how they code review so they can see the mental processes for it. Also encourage the juniors to ask questions in the PR. A simple "Why did you do it this way?" can be a valuable learning experience or even show the code author they did something ambiguous or hard to understand. When a junior makes a comment that causes a code change, they feel really good because it means they knew something!
For seniors you just have to assign them to PRs and then bug them until they get done unfortunately. There's a lot of arguments that you can use such as "Do you want to fix these code problems now, after hours when it breaks in production, or in a year when there's a bug and you have to work in the file?" and that can work a few times. That said, there's nothing you can do to make them want to just go through and look at PRs, though having a junior tag them on something they want a second opinion on in a PR can help a bit. Seniors also don't want to spend time on small details. Be sure your code has a linter, and also a static analyzer for best practices. Hook these things into the git process via git commit hooks, github checks, or the like. There are also apps like Codacy(slow but also offers education) or things like it that help a lot.
Finally for PRs that are just too big, its better to have a lot of small branches that lead into master.
If each of those branches were 3 files with 100 line changes, that would be 15 files with 1500 line changes! That's a lot for one person to go through accurately and would require someone to take an hour to do correctly. You're more likely to get people to PR if they can spend 15 minutes between tickets on them than if they need to set aside time.
But lastly, just ask your team individually, not in a meeting, why they don't PR currently. Is it time? Deadline pressure? Don't see value in it? Don't care? Don't think they have the expertise? If you ask in a meeting, some people's opinions get covered up or hidden. Once you know most of the reasons your team isn't doing it you can fix it.
Absolute worst case scenario, I think there are some code mentor services out there that offer PRs for like personal projects, perhaps you can outsource PRs? :P If they are PHP, JS, or Typescript let me know and I'll do a few for a nominal fee. :D I love doing PRs so I can see when the person understands the reason for the requested change and does it not because it's blocking the branch, but because they agree with the change.
Anyways, that should help you get well on the track to more PRs! There's a whole thing of PR etiquette and other things that aren't covered above, but that's not really relevant here. If you need clarification, or have other questions let me know and I'll try and help! Good luck!
Thanks for taking the time!
I'm not actually looking for specific advice, I was more interested in knowing how other teams out there are treating PRs and how their day-to-day work affects or is affected by PRs.
I totally agree that small details checks should be automated as much as possible through the use of a Continuous Integration pipeline so that peers can focus on the value that code change is meant to deliver.
BTW, I love the idea of pair-reviewing to lower the barrier to access for junior engineers.
Thanks very much for your feedback!
Haha, I guess I got a bit carried away in that case, glad I could be of some use regardless.
As for the CI to remove the small detail checking, I was talking even before that. In the actual project's git folder there is the hooks folder that you can use to prevent them from being able to push or commit without those checks running. githooks.com/ has some info on it. CI should for sure happen too(never trust the client, especially if you're the client), but it forces a bit of a long feedback cycle that kind of bites. :(
Knowledge transference. Developers are smart people, smart people want to know more, reviews are a way to spread that knowledge around.
At my last position we had a team agreement that no PR will sit for more than 24 hours without being viewed and no more than 48 hours (not counting weekends) without an approve/decline.
Myself, I would do PR's after lunch and at the end of the day. At this temp reviews would last no more than 15 minutes and would not be pressured.
YMMV, the important part is the team agreement.
I think the 24-48 hours agreement is pretty reasonable.
15 minutes review is a great achievement and hopefully it leads people to open short and manageable PRs.
I totally agree, team agreement is vital here.