Open Stack

> -----Original Message-----
> From: Flavio Percoco [mailto:flavio at redhat.com]
> Sent: Monday, August 26, 2013 11:41 AM
> To: OpenStack Development Mailing List
> Subject: Re: [openstack-dev] Code review study
> 
> 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-
> prac
> >>>>> tices-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-
> ca
> >>>> se-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.

[Gary Kotton] Agreed. The reviewers should use their discretion when it comes to the patch size. The flip side is that there are times when small patches based on on top of another fail to provide a complete picture of the change.

> 
> --
> @flaper87
> Flavio Percoco
> 
> _______________________________________________
> OpenStack-dev mailing list
> OpenStack-dev at lists.openstack.org
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev