[openstack-dev] Code review study
rbryant at redhat.com
Tue Aug 20 15:24:40 UTC 2013
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.
>>>> 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:
>>> 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:
>>> 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:
>> 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.
More information about the OpenStack-dev