Open Stack

> -----Original Message-----
> From: Robert Collins [mailto:robertc at robertcollins.net]
> Sent: 06 November 2013 22:08
> To: OpenStack Development Mailing List (not for usage questions)
> Subject: Re: [openstack-dev] Bad review patterns
> 
> On 6 November 2013 21:34, Radomir Dopieralski <openstack at sheep.art.pl>
> wrote:
> > Hello,
> 
> Secondly, this:
> 
> >
> > Leaving a mark.
> > ===============
> >
> > You review a change and see that it is mostly fine, but you feel that
> > since you did so much work reviewing it, you should at least find
> > *something* wrong. So you find some nitpick and -1 the change just so
> > that they know you reviewed it.
> 
> Thats indeed not cool. Perhaps a 0 with the nitpick. On the other hand,
> perhaps the nitpick actually matters. If it's a nitpick it's going to be what - a
> couple minutes to fix and push ?
> 
> ... I think the real concern is that by pushing it up again you go to the back of
> the queue for reviews, so maybe we should talk about that instead. We
> don't want backpressure on folk polishing a patch on request because of
> review latency.
> 
> > This is quite obvious. Just don't do it. It's OK to spend an hour
> > reviewing something, and then leaving no comments on it, because it's
> > simply fine, or because we had to means to test someting (see the
> > first pattern).
> 
> Core reviewers look for the /comments/ from people, not just the votes. A
> +1 from someone that isn't core is meaningless unless they are known to be
> a thoughtful code reviewer. A -1 with no comment is also bad, because it
> doesn't help the reviewee get whatever the issue is fixed.
> 
> It's very much not OK to spend an hour reviewing something and then +1
> with no comment: if I, and I think any +2er across the project see a patch that
> needs an hour of review, with a commentless +1, we'll likely discount the +1
> as being meaningful.
> 

I don't really get what you're saying here Rob.   It seems to me an almost inevitable
part of the review process that useful comments are going to be mostly negative.
If someone has invested that amount of effort because the patch is complex, or
It took then a while to work their way back into that part of the systems, etc, but 
having given the code careful consideration they decide it's good - do you want
comments in there saying "I really like your code", "Well done on fixing such a  
complex problem" or some such ?

I just don't see how you can use a lack or presence of positive feedback in a +1 as 
any sort of indication on the quality of that +1.   Unless you start asking reviewers
to précis the change in their own words to show that they understood it I don't 
really see how additional positive comments help in most cases.   Perhaps if you
have some specific examples of this it would help to clarify

Phil