[dev][all] note to non-core reviewers in all projects

Slawomir Kaplonski skaplons at redhat.com
Tue May 21 15:08:36 UTC 2019


> On 21 May 2019, at 16:47, Mohammed Naser <mnaser at vexxhost.com> wrote:
> On Tue, May 21, 2019 at 9:37 PM Brian Rosmaita
> <rosmaita.fossdev at gmail.com> wrote:
>> Hello everyone,
>> A recent spate of +1 reviews with no comments on patches has put me into
>> grumpy-old-man mode.
>> A +1 with no comments is completely useless (unless you have a review on
>> a previous patch set with comments that have been addressed by the
>> author).  I already know you're a smart person (you figured out how to
>> sign the CLA and navigate gerrit -- lots of people can't or won't do
>> that), but all your non-comment +1 tells me is that you are in favor of
>> the patch.  That doesn't give me any information, because I already know
>> that the author is in favor of the patch, so that makes two of you out
>> of about 1,168 reviewers.  That's not exactly a groundswell of support.
> I feel a bit different about this.
> Most cores probably leave a +2 with no comment to merge code, the implication
> that a non-core needs to add more comments to it seems a bit 'unfair'.
> I think this is something where we need to put our judgement in.  It may seem
> that we get a bunch of +1s, but if there is also other -1s that have value and
> comments, then I think it's okay that we get +1s.
> tl;dr: I don't think we should say "if you're going to give a +1,
> leave a comment
> why you agree", IMHO.

I agree with that. +1 without comment still can have some value.
But we should say “if you’re going to gibe a -1, please leave a comment”. That’s really important IMO.

>> When you post your +1, please leave a comment explaining why you
>> approve, or at least what in particular you looked at in the patch that
>> gave you a favorable impression.  This whole open source community thing
>> is a collaborative effort, so please collaborate!  You comment does not
>> have to be profound.  Even just saying that you checked that the release
>> note or docs on the patch rendered correctly in HTML is very helpful.
>> The same thing goes for leaving a -1 on a patch.  Don't just drop a -1
>> bomb with no explanation.  The kind of review that will put you on track
>> for becoming core in a project is what johnthetubaguy calls a
>> "thoughtful -1", that is, a negative review that clearly explains what
>> the problem is and points the author in a good direction to fix it.
>> That's all I have to say.  I now return to my normal sunny disposition.
>> cheers,
>> brian
> -- 
> Mohammed Naser — vexxhost
> -----------------------------------------------------
> D. 514-316-8872
> D. 800-910-1726 ext. 200
> E. mnaser at vexxhost.com
> W. http://vexxhost.com

Slawek Kaplonski
Senior software engineer
Red Hat

More information about the openstack-discuss mailing list