DEV Community

Discussion on: How do you incentivize developers to review Pull Requests?

Collapse
 
samuraiseoul profile image
Sophie The Lionhart

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.

Master -> feature -> subfeatureA -> subfeatureB -> minorFixA -> subfeatureC
Enter fullscreen mode Exit fullscreen mode

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!

Collapse
 
alediaferia profile image
Alessandro Diaferia

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!

Collapse
 
samuraiseoul profile image
Sophie The Lionhart

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. :(