Skip to content
This repository has been archived by the owner on Nov 9, 2017. It is now read-only.

Thinking Point: Will requiring 2 Collaborators' POV improve reviews? #144

Closed
refack opened this issue Jun 19, 2017 · 7 comments
Closed

Thinking Point: Will requiring 2 Collaborators' POV improve reviews? #144

refack opened this issue Jun 19, 2017 · 7 comments

Comments

@refack
Copy link

refack commented Jun 19, 2017

I can't believe this is coming from me, but I was thinking we should consider increasing the review requirements by a little bit.
From recent personal experience I find that reviews are better when there are at least two participants voicing opinions. So I suggest we require that for a PR to be land 2 Collaborators need to participate in the review. Either the the PR will need 2 approvals, or if the OP is a Collaborator, then OP + 1.
Although the more experienced non-Collaborator contributors can rationalize their decision processes very well, some of the newer contributors yield to reviews too quickly, IMHO leaving the review process a bit one sided.
Personally I feel that I do a better job when challenged. A little bit like in Socratic dialogue or 2000's style Pair programming

Something to think about?

@Trott
Copy link
Member

Trott commented Jun 19, 2017

@refack Can you point to a concrete example where additional reviews would have been useful? It would help me to have a real-world example I could consider.

@mcollina
Copy link
Member

I'm not exactly in favor of this, as most of the regression that I was involved in were signed off by multiple collaborators, so the bug was not easily spottable with a code review.

@targos
Copy link
Member

targos commented Jun 19, 2017

I also think we need concrete examples to make this kind of decisions.

@refack
Copy link
Author

refack commented Jun 19, 2017

First of all I'm not talking about regressions, I'm talking about optimizing quality.

I didn't want to ref PRs because I didn't want it to be construed as criticism of contributors, so if anybody that's reading this find their name referenced, this is not criticism!
IMHO contributions from users are the lifeblood of this project, and personally I consider them priceless (big, small, accepted, or rejected). As I see it, the Collaborators are here mostly to help things along...

I have a case from just now: nodejs/node#13723 - this could have landed, but a fresh pair of eyes brought a new idea nodejs/node#13723 (comment). (also before that for some reason I decided to review again and found nodejs/node#13723 (comment), which IMHO is worth discussing, and nodejs/node#13723 (comment))

@addaleax
Copy link
Member

@refack I think what your example shows is that it’s a good idea to apply 48/72-hour for non-trivial PRs, not necessarily anything relating to the # of reviewers.

@refack
Copy link
Author

refack commented Jun 19, 2017

@refack I think what your example shows is that it’s a good idea to apply 48/72-hour for non-trivial PRs, not necessarily anything relating to the # of reviewers.

I was just thinking about that too, extend the cooking time
image

@Trott
Copy link
Member

Trott commented Sep 8, 2017

I'm going to close this, but feel free to open another issue in a relevant active repository (TSC perhaps?) and include a link back to this issue if this is a subject that should receive continued attention.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants