Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Require multiple reviewers for pull requests

blog.github.com

Post reply on HN
Print view View original

21–30 of 37 posts

Re: Require multiple reviewers for pull requests

#21

Post by geekjock »

scarmig wrote: post #9

Having multiple reviewers is often bad . A diffusion of responsibility means no responsibility. If you have three reviewers, no one feels personally responsible for checking it line by line and thinking about the code in depth. Instead a cursory glance seems acceptable, because, after all, other people are looking at it.

It all depends on a particular teams workflow. I think it's great to see GitHub continuing to invest in workflow features like this.

I developed an app (https://pullreminders.com) that helps teams stay on top of open pull requests and in talking to customers, there is a huge demand for features and tools to support a better code review process.

(Disclaimer: I am the creator of Pull Reminders)

Re: Require multiple reviewers for pull requests

#22

Post by dunkelheit »

greglindahl wrote: post #16
dunkelheit wrote: post #14

Earlier quoted context omitted.

True. But having just one reviewer can be bad too: when two people review each other often, they often "collude" and start to rubber-stamp shitty code. Or the other extreme - they can go into an argument over some issue with no way to break the stalemate. Two reviewers seems optimal if you can afford it. Possibly with one person doing the bulk of the reviewing and the other more in the role of providing oversight and…

If you have people submitting shitty code for review, you've got bigger problems than needing a better review system.

Here are some of the reasons people will submit shitty code: they (even subconsciosly) think it will be somebody else's problem; they are junior; they don't fully understand the system they are changing; they don't feel polishing the code is valued. Many of these problems can be improved with a good review system.

I don't want to enter the people vs. systems debate but certainly a good system can help.

Re: Require multiple reviewers for pull requests

#23

Post by aurbano »

greglindahl wrote: post #19
bennofs wrote: post #18

Earlier quoted context omitted.

Welcome to open source maintainership

If you have new folks who aren't fully socialized to the project yet, having multiple reviewers is much worse than having a couple of people specifically individually coaching new would-be committers. I use a coaching approach for new hires at my startups.

Could you describe your coaching approach? That sounds interesting, and is often a difficult problem to solve

Re: Require multiple reviewers for pull requests

#24

Post by koolba »

bennofs wrote: post #18
greglindahl wrote: post #16

Earlier quoted context omitted.

If you have people submitting shitty code for review, you've got bigger problems than needing a better review system.

Welcome to open source maintainership

Maintaining a popular open source project is a great way to learn to politely say “fuck you this is crap”.

It’s important to be consistent with your online voice though. If you’re always blunt and terse then, generally, people won’t take it badly. But if you slide between extraneous gestures and out right profanity then eventually someone will take it personally.

Re: Require multiple reviewers for pull requests

#25

Post by table-delete »

greglindahl wrote: post #16
dunkelheit wrote: post #14

Earlier quoted context omitted.

True. But having just one reviewer can be bad too: when two people review each other often, they often "collude" and start to rubber-stamp shitty code. Or the other extreme - they can go into an argument over some issue with no way to break the stalemate. Two reviewers seems optimal if you can afford it. Possibly with one person doing the bulk of the reviewing and the other more in the role of providing oversight and…

If you have people submitting shitty code for review, you've got bigger problems than needing a better review system.

> you've got bigger problems than needing a better review system

I'll set aside the fallacious premise that everybody can stack their team with 10x engineers. Some of us actually work with interns and juniors.

That being said, your comment doesn't invalidate the need for a better review system. Software quality isn't defined by any one tool or process; the product is the end result of a chain of tools and processes...

Re: Require multiple reviewers for pull requests

#26

Post by scotu »

dunkelheit wrote: post #14
scarmig wrote: post #9

Having multiple reviewers is often bad . A diffusion of responsibility means no responsibility. If you have three reviewers, no one feels personally responsible for checking it line by line and thinking about the code in depth. Instead a cursory glance seems acceptable, because, after all, other people are looking at it.

True. But having just one reviewer can be bad too: when two people review each other often, they often "collude" and start to rubber-stamp shitty code. Or the other extreme - they can go into an argument over some issue with no way to break the stalemate. Two reviewers seems optimal if you can afford it. Possibly with one person doing the bulk of the reviewing and the other more in the role of providing oversight and…

I think 1 reviewer works better in general, but multiple reviewers could be made to work if the reviews and the reviewers were separate and unaware of the other?

Re: Require multiple reviewers for pull requests

#27

Post by erikpukinskis »

scarmig wrote: post #9

Having multiple reviewers is often bad . A diffusion of responsibility means no responsibility. If you have three reviewers, no one feels personally responsible for checking it line by line and thinking about the code in depth. Instead a cursory glance seems acceptable, because, after all, other people are looking at it.

I’ve lived in a lot of collective houses. I’ve seen a lot of chore systems come and go.

What I’ve decided is that it’s folly to try to enforce accountability. A good system provides visibility, that’s it. Who did what? Document it, but allow implicit social process to handle enforcement. Hard rules just lead to people cooking the books, or opting out of the system entirely.

I feel the same about code review. Document who reviewed. But it’s up to individuals to take responsibility for the outcome. And at the end of the day it’s the committer’s code. It’s their name in the git history.

Re: Require multiple reviewers for pull requests

#28

Post by greglindahl »

aurbano wrote: post #23
greglindahl wrote: post #19

Earlier quoted context omitted.

If you have new folks who aren't fully socialized to the project yet, having multiple reviewers is much worse than having a couple of people specifically individually coaching new would-be committers. I use a coaching approach for new hires at my startups.

Could you describe your coaching approach? That sounds interesting, and is often a difficult problem to solve

Well, the basic idea for coaching a brand-new employee is that you hired a perfectly smart person for the job, but your company has a strong coding culture with a particular style that the new engineer isn't familiar with. So it's a big part of on-boarding to not only teach this smart engineer the crazy weirdnesses of your system, but also teach them (what they will perceive as) the oddities of your corporate coding style.

It can be good to do this outside of the public eye, so the initial cycle of "wtf, why is this considered standard" followed by "well, it's not so much that this is the best way, but it's more likely that we'll be able to understand each other's code if we all do it one way" can be done in private.

Once the engineer is well-versed in the system, they are unleashed to quietly subvert it, if that is their will :-)

Re: Require multiple reviewers for pull requests

#29

Post by wumpus »

table-delete wrote: post #25
greglindahl wrote: post #16

Earlier quoted context omitted.

If you have people submitting shitty code for review, you've got bigger problems than needing a better review system.

> you've got bigger problems than needing a better review system I'll set aside the fallacious premise that everybody can stack their team with 10x engineers. Some of us actually work with interns and juniors. That being said, your comment doesn't invalidate the need for a better review system. Software quality isn't defined by any one tool or process; the product is the end result of a chain of tools and processes..…

I'm all for good review systems. But any review system that labels a large fraction of the submitted code as very deficient indicates that the solution is to act before review.

And I've coded with (and hired) interns and juniors, so it's not just a selection effect. In fact, the worst code I've seen came from a senior engineer, with strong opinions, who wrote the world's most blecherous code. We fired him, but not soon enough.

Re: Require multiple reviewers for pull requests

#30

Post by greglindahl »

booleandilemma wrote: post #15
scarmig wrote: post #9

Having multiple reviewers is often bad . A diffusion of responsibility means no responsibility. If you have three reviewers, no one feels personally responsible for checking it line by line and thinking about the code in depth. Instead a cursory glance seems acceptable, because, after all, other people are looking at it.

Maybe don’t let the reviewers know there are other reviewers? Like a BCC feature.

Yeah, over the years no one would possibly notice. Especially smart engineers.
Post reply on HN