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

Bogdan Dobrelya bdobreli at redhat.com
Tue May 21 15:23:38 UTC 2019


On 21.05.2019 17:08, Slawomir Kaplonski wrote:
> Hi,
> 
>> 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.

+1

> 
>>
>>> 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
> 
> 


-- 
Best regards,
Bogdan Dobrelya,
Irc #bogdando



More information about the openstack-discuss mailing list