[openstack-dev] Bad review patterns

David Stanek dstanek at dstanek.com
Thu Nov 7 02:59:04 UTC 2013


On Wed, Nov 6, 2013 at 7:21 PM, Day, Phil <philip.day at hp.com> wrote:

> >
> > 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.
> >
> > 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).
> >
> >
>
> Another one that comes into this category is adding a -1 which just says
> "I agree with
> the other -1's in here".   If you have some additional perspective and can
> expand on
> it then that's fine - otherwise it adds very little and is just review
> count chasing.
>
> It's an unfortunate consequence of counting and publishing review stats
> that having
> such a measure will inevitable also drive behavour.


I agree that having no comment with a +1 is OK.  If I think the code looks
good I'll +1 it.  If I think the code could be better I'll -1 it and leave
comments.

I don't think that leaving a -1 with a 'what they said' comment is a bad
thing.  As the developer writing the patch it's helpful to know there is a
consensus and it's not just one person requesting a change.

-- 
David
blog: http://www.traceback.org
twitter: http://twitter.com/dstanek
www: http://dstanek.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstack.org/pipermail/openstack-dev/attachments/20131106/af16fa9a/attachment.html>


More information about the OpenStack-dev mailing list