Open Stack

On 20/08/13 11:24 -0400, Russell Bryant wrote:
>On 08/20/2013 11:08 AM, Daniel P. Berrange wrote:
>> On Tue, Aug 20, 2013 at 04:02:12PM +0100, Mark McLoughlin wrote:
>>> On Tue, 2013-08-20 at 11:26 +0100, Mark McLoughlin wrote:
>>>> On Thu, 2013-08-15 at 14:12 +1200, Robert Collins wrote:
>>>>> This may interest data-driven types here.
>>>>>
>>>>> https://www.ibm.com/developerworks/rational/library/11-proven-practices-for-peer-review/
>>>>>
>>>>> Note specifically the citation of 200-400 lines as the knee of the review
>>>>> effectiveness curve: that's lower than I thought - I thought 200 was
>>>>> clearly fine - but no.
>>>>
>>>> The full study is here:
>>>>
>>>> http://support.smartbear.com/resources/cc/book/code-review-cisco-case-study.pdf
>>>>
>>>> This is an important subject and I'm glad folks are studying it, but I'm
>>>> sceptical about whether the "Defect density vs LOC" is going to help us
>>>> come up with better guidelines than we have already.
>>>>
>>>> Obviously, a metric like LOC hides some serious subtleties. Not all
>>>> changes are of equal complexity. We see massive refactoring patches
>>>> (like s/assertEquals/assertEqual/) that are actually safer than gnarly,
>>>> single-line, head-scratcher bug-fixes. The only way the report addresses
>>>> that issue with the underlying data is by eliding >10k LOC patches.
>>>>
>>>> The "one logical change per commit" is a more effective guideline than
>>>> any LOC based guideline:
>>>>
>>>> https://wiki.openstack.org/wiki/GitCommitMessages#Structural_split_of_changes
>>>>
>>>> IMHO, the number of distinct logical changes in a patch has a more
>>>> predictable impact on review effectiveness than the LOC metric.
>>>
>>> Wow, I didn't notice Joe had started to enforce that here:
>>>
>>>   https://review.openstack.org/41695
>>>
>>> and the exact example I mentioned above :)
>>>
>>> We should not enforce rules like this blindly.
>>
>> Agreed, lines of code is a particularly poor metric for evaluating
>> commits on.
>
>Agreed, I would _strongly_ prefer no enforcement around LOC.  It's just
>not the right metric to be looking at for a hard rule.
>

Agreed. I think we should focus on other things like per feature
patches, which make more sense. Huge patches can be split in several
ones - most of the time - which will implicitly enforce a LOC limit
but will let patches like s/assertEquals/assertEqual/ land, which make
sense to me.

This should be evaluated in a case-by-case basis.

-- 
@flaper87
Flavio Percoco