Re: [openstack-dev] [all] add cyclomatic complexity check to pep8 target

2014-10-20 Thread Ed Leafe
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 10/16/2014 11:09 PM, Joe Gordon wrote: First step in fixing this, put a cap on it: https://review.openstack.org/129125 To get the maximums down to more reasonable levels, I have two patches that bring the libvirt driver _get_guest_config() from

Re: [openstack-dev] [all] add cyclomatic complexity check to pep8 target

2014-10-17 Thread Clint Byrum
Excerpts from Mike Spreitzer's message of 2014-10-16 22:24:30 -0700: I like the idea of measuring complexity. I looked briefly at `python -m mccabe`. It seems to measure each method independently. Is this really fair? If I have a class with some big methods, and I break it down into

Re: [openstack-dev] [all] add cyclomatic complexity check to pep8 target

2014-10-17 Thread Daniel P. Berrange
On Fri, Oct 17, 2014 at 03:03:43PM +1100, Michael Still wrote: I think nova wins. We have: ./nova/virt/libvirt/driver.py:3736:1: C901 'LibvirtDriver._get_guest_config' is too complex (67) IMHO this tool is of pretty dubious value. I mean that function is long for sure, but it is by no means

Re: [openstack-dev] [all] add cyclomatic complexity check to pep8 target

2014-10-17 Thread Matthew Gilliard
I like measuring code metrics, and I definitely support Joe's change here. I think of McCabe complexity as a proxy for testability and readability of code, both of which are IMO actual real problems in the nova codebase. If you are an experienced openstack dev you might find the code easy to move

Re: [openstack-dev] [all] add cyclomatic complexity check to pep8 target

2014-10-17 Thread Chris Dent
On Fri, 17 Oct 2014, Daniel P. Berrange wrote: IMHO this tool is of pretty dubious value. I mean that function is long for sure, but it is by no means a serious problem in the Nova libvirt codebase. The stuff it complains about in the libvirt/config.py file is just incredibly stupid thing to

Re: [openstack-dev] [all] add cyclomatic complexity check to pep8 target

2014-10-17 Thread Jay Pipes
On 10/17/2014 05:10 AM, Chris Dent wrote: On Fri, 17 Oct 2014, Daniel P. Berrange wrote: IMHO this tool is of pretty dubious value. I mean that function is long for sure, but it is by no means a serious problem in the Nova libvirt codebase. The stuff it complains about in the libvirt/config.py

Re: [openstack-dev] [all] add cyclomatic complexity check to pep8 target

2014-10-17 Thread Ian Cordasco
I would also advise pinning the version of mccabe we’re using. Mccabe was originally a proof-of-concept script that Ned Batchelder wrote and which Tarek Ziade vendored into Flake8. After we split it out in v2 of Flake8, we’ve found several (somewhat serious) reporting problems with the tool.

Re: [openstack-dev] [all] add cyclomatic complexity check to pep8 target

2014-10-16 Thread Joe Gordon
On Thu, Oct 16, 2014 at 8:11 PM, Angus Salkeld asalk...@mirantis.com wrote: Hi all I came across some tools [1] [2] that we could use to make sure we don't increase our code complexity. Has anyone had any experience with these or other tools? Flake8 (and thus hacking) has built in

Re: [openstack-dev] [all] add cyclomatic complexity check to pep8 target

2014-10-16 Thread Dolph Mathews
I ran this tool on Keystone and liked the results - the two methods that ranked D should certainly be refactored. It also matched a few methods in openstack/common. But flake8 supports complexity checking already, using mccabe. Just enable complexity checking with: $ flake8 --max-complexity 20

Re: [openstack-dev] [all] add cyclomatic complexity check to pep8 target

2014-10-16 Thread Morgan Fainberg
I agree we should use flake8 built-in if at all possible. I complexity checking will definitely help us in the long run keeping code maintainable. +1 from me.  — Morgan Fainberg On October 16, 2014 at 20:45:35, Joe Gordon (joe.gord...@gmail.com) wrote: On Thu, Oct 16, 2014 at 8:11 PM, Angus

Re: [openstack-dev] [all] add cyclomatic complexity check to pep8 target

2014-10-16 Thread Michael Still
I think nova wins. We have: ./nova/virt/libvirt/driver.py:3736:1: C901 'LibvirtDriver._get_guest_config' is too complex (67) Michael On Fri, Oct 17, 2014 at 2:45 PM, Dolph Mathews dolph.math...@gmail.com wrote: I ran this tool on Keystone and liked the results - the two methods that ranked D

Re: [openstack-dev] [all] add cyclomatic complexity check to pep8 target

2014-10-16 Thread Angus Salkeld
On Fri, Oct 17, 2014 at 1:40 PM, Joe Gordon joe.gord...@gmail.com wrote: On Thu, Oct 16, 2014 at 8:11 PM, Angus Salkeld asalk...@mirantis.com wrote: Hi all I came across some tools [1] [2] that we could use to make sure we don't increase our code complexity. Has anyone had any

Re: [openstack-dev] [all] add cyclomatic complexity check to pep8 target

2014-10-16 Thread Joe Gordon
On Thu, Oct 16, 2014 at 8:53 PM, Morgan Fainberg morgan.fainb...@gmail.com wrote: I agree we should use flake8 built-in if at all possible. I complexity checking will definitely help us in the long run keeping code maintainable. Well this is scary: ./nova/virt/libvirt/driver.py:3736:1: C901

Re: [openstack-dev] [all] add cyclomatic complexity check to pep8 target

2014-10-16 Thread John Griffith
On Thu, Oct 16, 2014 at 10:09 PM, Joe Gordon joe.gord...@gmail.com wrote: On Thu, Oct 16, 2014 at 8:53 PM, Morgan Fainberg morgan.fainb...@gmail.com wrote: I agree we should use flake8 built-in if at all possible. I complexity checking will definitely help us in the long run keeping code

Re: [openstack-dev] [all] add cyclomatic complexity check to pep8 target

2014-10-16 Thread Angus Salkeld
On Fri, Oct 17, 2014 at 2:09 PM, Joe Gordon joe.gord...@gmail.com wrote: On Thu, Oct 16, 2014 at 8:53 PM, Morgan Fainberg morgan.fainb...@gmail.com wrote: I agree we should use flake8 built-in if at all possible. I complexity checking will definitely help us in the long run keeping code

Re: [openstack-dev] [all] add cyclomatic complexity check to pep8 target

2014-10-16 Thread Steve Kowalik
On 17/10/14 15:23, Angus Salkeld wrote: Thanks for that, I mostly copied your patch for Heat: https://review.openstack.org/#/c/129126/ I wish it would print out the value for all the modules tho', that is what is nice about radon. It's a little horrible, but we can do the same thing:

Re: [openstack-dev] [all] add cyclomatic complexity check to pep8 target

2014-10-16 Thread Mike Spreitzer
I like the idea of measuring complexity. I looked briefly at `python -m mccabe`. It seems to measure each method independently. Is this really fair? If I have a class with some big methods, and I break it down into more numerous and smaller methods, then the largest method gets smaller,

Re: [openstack-dev] [all] add cyclomatic complexity check to pep8 target

2014-10-16 Thread Angus Salkeld
On Fri, Oct 17, 2014 at 3:24 PM, Mike Spreitzer mspre...@us.ibm.com wrote: I like the idea of measuring complexity. I looked briefly at `python -m mccabe`. It seems to measure each method independently. Is this really fair? I think it is a good starting point. You still need to write

Re: [openstack-dev] [all] add cyclomatic complexity check to pep8 target

2014-10-16 Thread Michael Davies
On Fri, Oct 17, 2014 at 2:39 PM, Joe Gordon joe.gord...@gmail.com wrote: First step in fixing this, put a cap on it: http://goog_106984861 https://review.openstack.org/129125 Thanks Joe - I've just put up a similar patch for Ironic: https://review.openstack.org/129132 -- Michael Davies