Open Stack

On 7 November 2013 13:15, Day, Phil <philip.day at hp.com> wrote:
>
>

>>
>> 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 ?

Negative comments are fine: I was saying that statistically, having an
hour-long patch (which BTW puts it waaay past the diminishing returns
on patch size tradeoff) to review and then having nothing at all to
say about it is super suspect. Sure, having nothing to say on a 10
line patch - hit a +1, move on. But something that takes an hour to
review? I'd expect at minimum the thing to prompt some suggestions
*even while it gets a +1*.

> 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

It might even be negative feedback. Like - "this patch is correct but
the fact it had to be done as one patch shows our code structure here
is wonky. If you could follow up with something to let us avoid future
mammoth patches, that would be awesome."

Again, I'm talking about hour long reviews, which is the example Radomir gave.

Another way of presenting what I'm saying is this: Code reviews are a
dialogue. How often in a discussion with a live human would you have
no feedback at all, if they were reading a speech to you. You might go
'that was a great speech' (+2) and *still* have something to add. As
an observer given a very long speech, and an audience, I'd suspect
folk went to sleep if they didn't have commentary :)


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