-
Notifications
You must be signed in to change notification settings - Fork 376
Require approval from 2 reviewers before merging #3353
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
evindj
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as per agreement in the discussion thread.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is unfortunate but this is must required now specially
after repeated occurence :
1/ #3207 (comment)
2/#3268 (comment)
|
@flyrain no objection from you about this one ? It seems we have already a consensus from several contributors. |
|
To carry on my thoughts on the dev mailing discussion, I don't think enforcing 2 reviewers on PRs solves the problem, meanwhile it can slow down the review process, esp. on trivial PRs. But I will not block it if people want to give it a try. |
|
@flyrain I got your point. I'm not hard pushing for this one, but more as an attempt to involve more reviewers. @adutra @MonkeyCanCode @singhpk234 @dimas-b @evindj @adam-christian-software what do you guys think ? Should we try this ? I'm fine to close this PR if you think it won't help (I'm not very convinced either 😄 ). |
|
(Casting my objection on the dev-ML discussion here:) |
|
OK, so, we don't have consensus. Let's close this one, we can revisit later if needed. |
|
For posterity I'm with @flyrain here: I don't really think 2 reviewers solves the original issue, but I wouldn't oppose the idea either. |
No description provided.