Open Stack

On 21/05/2019 15:47, Mohammed Naser 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.

If you have no history reviewing code in the project, yes, you do need
to leave a comment. I see patches that get pushed up, and then a flurry
of +1's with no coments arrive from people that don't have a history of
good reviews, I will discount them.

I am assuming that before someone +1s they do some sort of check, be
that reading the diff line by line and agreeing with the code +
direction of the patch, or running devstack and seeing if it ran, and
provided the fix or feature - if you have just say what you did.

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


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.openstack.org/pipermail/openstack-discuss/attachments/20190521/9a8f3c59/attachment.sig>