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
_______________________________________________
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
