Open Stack

On 06/11/13 23:07, Robert Collins wrote:
> On 6 November 2013 21:34, Radomir Dopieralski <openstack at sheep.art.pl> wrote:

> [...] 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.

This is of course a gut feeling thing, and different people have
different thresholds for when to deduplicate code. I came up with that
pattern for myself, because my feelings about the code often didn't
match the feelings of the rest of the team. Please note that the fact
that I notice a bad pattern in my review doesn't immediately mean I
shouldn't do it -- just that it is suspect and I should rethink it. So,
if it's a blatant copy-paste of code, of course I will point it out. If
the patch includes code that could be easily replaced by an existing
utility function, that the author might not know about, of course I will
point it out. But if the code is similar to some other code in some
other part of the project, I will try to treat it the same way as when I
spot a bug that is not related to the patch during the review -- either
follow up with a patch of my own, or report a bug if I don't have time
for this (this has a nice side effect of producing the low-hanging-fruit
bugs that teach refactoring to newcomers). I think it's important to
remember that you can always commit to that project, so not all changes
need to be mde by that single author (especially in OpenStack, where you
have the marvelous option of amending someone else's patch).

[...]
>> 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.

That is a very good point! We want reviews that have useful comments in
them, so it is indeed a very good rule of thumb to always ask yourself
"Is this comment that I am posting helpful?". What I meant here is that
if I do a review, and even after spending a lot of time on it, can't
come up with anything that would actually help the author make it a
better patch, then it is fine to not post anything at all, including no
score. I suppose it would be nice to +1 it if you find it basically
fine, but we didn't have separate +1s and +2s in my previous project.

-- 
Radomir Dopieralski