Open Stack

On 6 November 2013 21:34, Radomir Dopieralski <openstack at sheep.art.pl> wrote:
> Hello,
>
> I'm quite new in the OpenStack project, but I love it already. What is
> especially nifty is the automated review system -- I'm really impressed.
> I'm coming from a project in which we also did reviews of every change
> -- although it was mostly manual, and just one review was enough to
> merge -- and at some point in that project I noticed that it is very
> easy to give reviews that are unhelpful, frustrating and just get in the
> way of the actual work. I started paying attention to how I am reviewing
> code, and I managed to come up with several patterns that are bad. Once
> I know the patterns, it's easier to recognize when I'm doing something
> wrong and rethink the review. I would like to share the patterns that I
> noticed.

Thank you for this, very cool. I have two nits ;). Firstly, things
like code duplication is a sliding scale, and I think it's ok for a
reviewer to say 'these look similar, give it a go please'. If the
reviewee tries, and it's no good - thats fine. But saying 'hey, I
won't even try' - thats not so good. Many times we get drive-by
patches and no follow up, so there is a learnt response to require
full satisfaction with each patch that comes in. Regular contributors
get more leeway here I think.

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.

-Rob

-- 
Robert Collins <rbtcollins at hp.com>
Distinguished Technologist
HP Converged Cloud