On 11/07/2013 01:56 PM, James Bottomley wrote: > On Thu, 2013-11-07 at 00:21 +0000, Day, Phil 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. > > Perhaps a source of the problem is early voting. Feeling pressure to > add a +/-1 (or even 2) without fully asking what's going on in the code > leads to premature adjudication. > > For instance, I have to do a lot of reviews in SCSI; I'm a subject > matter expert, so I do recognise some bad coding patterns and ask for > them to be changed unconditionally, but a lot of the time I don't > necessarily understand why the code was done in the way it was, so I > ask. Before I get the answer I'm not really qualified to judge the > merits.
+1 to this.
Realistically if I have questions in the code and am unsure how I'd
score it I'll often leave a 0 review with questions inline to help me
understand. I consider this generally good form, and it's helpful to
everyone in the learning process about the code.
-Sean
--
Sean Dague
http://dague.net
signature.asc
Description: OpenPGP digital signature
_______________________________________________ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
