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 67 down to 40,
and nova/tests/db/fakes.py stub_out_db_network_api from 57 down to 2.

https://review.openstack.org/#/c/129325/
https://review.openstack.org/#/c/129648/

These are just refactorings that will the maximum set in
https://review.openstack.org/#/c/129125/ to be set to something a little
saner than 68!


- -- Ed Leafe
-BEGIN PGP SIGNATURE-
Version: GnuPG v2.0.14 (GNU/Linux)

iQIcBAEBAgAGBQJURUVUAAoJEKMgtcocwZqLwXgQAKEdX/Ir5ud6DJIwyBgbBwIW
ScFcfIuNQZmZ5OWx/YGsiTBo9bkuARmknOcQm0TwUtXvKJkjPVzFl+6oSYov+fzT
aF4qsk7tAJaDbCi06YEAxGfy2yqQKO6D5FayTBwXUmMwg1IUQhZXMLhPlkbu65F1
WQDLJeGLXyoBbBQetUgy39jAHeCseIEe0IqR0PWgG/CVoGwRz96orNXCzFFvZPQT
nJ5qN3NFvwmBUha98Axu5rnt/k+Ua4rnbyY4dycn0jcGxFAgt7E086NgLKMLSBMv
ncgFfUzUfTr+dhQcaAaVCj63sgNRAtZIrHwniYsXtVo7o4O1QrKE9gnT0haWmUFb
dkwoNT/tYFVLFIkgIhucoWUf/6csyB4Sm2xd3jFAIPLa+Zmxc6jvBD/YWthjSlHu
rBSSujqAJxJ5jldh4YiCJMyXXp+W6t4wwjV0JDhNYDWyx2vHScp5H2Nf5DjTufad
Jm85h7GsaYLQcqUuaoBqZnwXzLan76DfPoSnfJsMSR3HmNSutMFumb7vEHDoO9Ib
keIGun1SKvM/Th5d+QJ0CMIcxJRD2HHlOtE2te/SB6YVlbTzxxZuykbmEhE/8nHW
ir/JaTCpp96zK0uLScNW3RXeVBymU2YyK4UnL2co0VzLUK/OUhP1FKXmul3aBYp+
JHvMZy8/LsqFr75v20cs
=dszR
-END PGP SIGNATURE-

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


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 
 more numerous and smaller methods, then the largest method gets smaller, 
 but the number of methods gets larger.  A large number of methods is 
 itself a form of complexity.  It is not clear to me that said re-org has 
 necessarily made the class easier to understand.  I can also break one 
 class into two, but it is not clear to me that the project has necessarily 
 become easier to understand.  While it is true that when you truly make a 
 project easier to understand you sometimes break it into more classes, it 
 is also true that you can do a bad job of re-organizing a set of classes 
 while still reducing the size of the largest method.  Has the McCabe 
 metric been evaluated on Python projects?  There is a danger in focusing 
 on what is easy to measure if that is not really what you want to 
 optimize.
 
 BTW, I find that one of the complexity issues for me when I am learning 
 about a Python class is doing the whole-program type inference so that I 
 know what the arguments are.  It seems to me that if you want to measure 
 complexity of Python code then something like the complexity of the 
 argument typing should be taken into account.
 

Fences don't solve problems. Fences make it harder to cause problems.

Of course you can still do the wrong thing and make the code worse. But
you can't do _this_ wrong thing without asserting why you need to.

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


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 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 highlight.

We've got plenty of big problems that need addressing in the OpenStack
codebase and I don't see this tool highlighting any of them. Better to
have people focus on solving actual real problems we have than trying
to get some arbitrary code analysis score to hit a magic value.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


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 around, but large and complex functions are
difficult for beginners to grok.

As an exercise, I took the method in libvirt/config.py and removed
everything except the flow-control keywords (ie the things that affect
the McCabe complexity): http://paste.openstack.org/show/121589/ - I
would find it difficult to hold all that in my head at once. It's
possible to argue that this is a false-positive, but my experience is
that this tool finds code which need improvement.

That said, these should be descriptive metrics rather than
prescriptive targets. There are products which chart a codebase's
evolution over time, such as www.sonarsource.com, which are really
great for provoking thought and conversation about code quality. Now
I'm interested, I'll have a look into it.

  Matthew

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


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 highlight.


I find a lot of the OpenStack code very hard to read. If it is very
hard to read it is very hard to maintain, whether that means fix or
improve.

That said, the value I see in these kinds of tools is not
specifically in preventing complexity, but in providing entry points
for people who want to fix things. You don't know where to start
(because you haven't yet got the insight or experience): run
flake8 or pylint or some other tools, do what it tells you. In the
process you will:

* learn more about the code
* probably find bugs
* make an incremental improvement to something that needs it

--
Chris Dent tw:@anticdent freenode:cdent
https://tank.peermore.com/tanks/cdent

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


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 file is
just incredibly stupid thing to highlight.


I find a lot of the OpenStack code very hard to read. If it is very
hard to read it is very hard to maintain, whether that means fix or
improve.


Exactly, ++.


That said, the value I see in these kinds of tools is not
specifically in preventing complexity, but in providing entry points
for people who want to fix things. You don't know where to start
(because you haven't yet got the insight or experience): run
flake8 or pylint or some other tools, do what it tells you. In the
process you will:

* learn more about the code
* probably find bugs
* make an incremental improvement to something that needs it


Agreed.

-jay

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


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.
Currently the package owner on PyPI hasn’t granted me permissions to
release a new version of the package, but we have several fixes in the
repository: https://github.com/flintwork/mccabe. The changes are somewhat
drastic but they should reduce the average function/method’s complexity by
1 or 2 points. I’m going to bother Florent again to give me permission to
release the package since it has been far too long since a release has
been cut.

For what it’s worth, Florent doesn’t pay close attention to GitHub
notifications so chiming in (or creating) issues on mccabe to release a
new version will only spam *me*. So please don’t pile on to anything
existing or create a new one.

Cheers,
Ian

On 10/17/14, 12:39 AM, Michael Davies mich...@the-davies.net wrote:

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_106984861https://review.openstack.org/129125







Thanks Joe - I've just put up a similar patch for Ironic:
https://review.openstack.org/129132 https://review.openstack.org/129132
 


-- 
Michael Davies   mich...@the-davies.net
Rackspace Australia


___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


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

2014-10-16 Thread Angus Salkeld
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?

radon is the underlying reporting tool and xenon is a monitor - meaning
it will fail if a threshold is reached.

To save you the time:
radon cc -nd heat
heat/engine/stack.py
M 809:4 Stack.delete - E
M 701:4 Stack.update_task - D
heat/engine/resources/server.py
M 738:4 Server.handle_update - D
M 891:4 Server.validate - D
heat/openstack/common/jsonutils.py
F 71:0 to_primitive - D
heat/openstack/common/config/generator.py
F 252:0 _print_opt - D
heat/tests/v1_1/fakes.py
M 240:4 FakeHTTPClient.post_servers_1234_action - F

It ranks the complexity from A (best) upwards, the command above (-nd) says
only show D or worse.
If you look at these methods they are getting out of hand and are  becoming
difficult to understand.
I like the idea of having a threshold that says we are not going to just
keep adding to the complexity
of these methods.

This can be enforced with:
xenon --max-absolute E heat
ERROR:xenon:block heat/tests/v1_1/fakes.py:240 post_servers_1234_action
has a rank of F

[1] https://pypi.python.org/pypi/radon
[2] https://pypi.python.org/pypi/xenon

If people are open to this, I'd like to add these to the test-requirements
and trial this in Heat
(as part of the pep8 tox target).

Regards
Angus
___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


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 McCabe Complexity checking.

flake8 --select=C --max-complexity 10

https://github.com/flintwork/mccabe
http://flake8.readthedocs.org/en/latest/warnings.html

Example on heat: http://paste.openstack.org/show/121561
Example in nova (max complexity of 20):
http://paste.openstack.org/show/121562



 radon is the underlying reporting tool and xenon is a monitor - meaning
 it will fail if a threshold is reached.

 To save you the time:
 radon cc -nd heat
 heat/engine/stack.py
 M 809:4 Stack.delete - E
 M 701:4 Stack.update_task - D
 heat/engine/resources/server.py
 M 738:4 Server.handle_update - D
 M 891:4 Server.validate - D
 heat/openstack/common/jsonutils.py
 F 71:0 to_primitive - D
 heat/openstack/common/config/generator.py
 F 252:0 _print_opt - D
 heat/tests/v1_1/fakes.py
 M 240:4 FakeHTTPClient.post_servers_1234_action - F

 It ranks the complexity from A (best) upwards, the command above (-nd)
 says only show D or worse.
 If you look at these methods they are getting out of hand and are
 becoming difficult to understand.
 I like the idea of having a threshold that says we are not going to just
 keep adding to the complexity
 of these methods.

 This can be enforced with:
 xenon --max-absolute E heat
 ERROR:xenon:block heat/tests/v1_1/fakes.py:240 post_servers_1234_action
 has a rank of F

 [1] https://pypi.python.org/pypi/radon
 [2] https://pypi.python.org/pypi/xenon

 If people are open to this, I'd like to add these to the test-requirements
 and trial this in Heat
 (as part of the pep8 tox target).


I think the idea of gating on complexity is a great idea and would like to
see nova adopt this as well. But why not just use flake8's built in stuff?



 Regards
 Angus

 ___
 OpenStack-dev mailing list
 OpenStack-dev@lists.openstack.org
 http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


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

It seems to function about the same as radon + xenon, where a D in that
tool is about a --max-complexity of 22. And flake8 obeys our existing tox
directives (such as ignoring openstack/common).

+1 for enabling complexity checking though.

On Thu, Oct 16, 2014 at 10: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?

 radon is the underlying reporting tool and xenon is a monitor - meaning
 it will fail if a threshold is reached.

 To save you the time:
 radon cc -nd heat
 heat/engine/stack.py
 M 809:4 Stack.delete - E
 M 701:4 Stack.update_task - D
 heat/engine/resources/server.py
 M 738:4 Server.handle_update - D
 M 891:4 Server.validate - D
 heat/openstack/common/jsonutils.py
 F 71:0 to_primitive - D
 heat/openstack/common/config/generator.py
 F 252:0 _print_opt - D
 heat/tests/v1_1/fakes.py
 M 240:4 FakeHTTPClient.post_servers_1234_action - F

 It ranks the complexity from A (best) upwards, the command above (-nd)
 says only show D or worse.
 If you look at these methods they are getting out of hand and are
 becoming difficult to understand.
 I like the idea of having a threshold that says we are not going to just
 keep adding to the complexity
 of these methods.

 This can be enforced with:
 xenon --max-absolute E heat
 ERROR:xenon:block heat/tests/v1_1/fakes.py:240 post_servers_1234_action
 has a rank of F

 [1] https://pypi.python.org/pypi/radon
 [2] https://pypi.python.org/pypi/xenon

 If people are open to this, I'd like to add these to the test-requirements
 and trial this in Heat
 (as part of the pep8 tox target).

 Regards
 Angus

 ___
 OpenStack-dev mailing list
 OpenStack-dev@lists.openstack.org
 http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


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 Salkeld  
 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 McCabe Complexity checking.
  
 flake8 --select=C --max-complexity 10
  
 https://github.com/flintwork/mccabe
 http://flake8.readthedocs.org/en/latest/warnings.html
  
 Example on heat: http://paste.openstack.org/show/121561
 Example in nova (max complexity of 20):
 http://paste.openstack.org/show/121562
  
  
 
  radon is the underlying reporting tool and xenon is a monitor - meaning
  it will fail if a threshold is reached.
 
  To save you the time:
  radon cc -nd heat
  heat/engine/stack.py
  M 809:4 Stack.delete - E
  M 701:4 Stack.update_task - D
  heat/engine/resources/server.py
  M 738:4 Server.handle_update - D
  M 891:4 Server.validate - D
  heat/openstack/common/jsonutils.py
  F 71:0 to_primitive - D
  heat/openstack/common/config/generator.py
  F 252:0 _print_opt - D
  heat/tests/v1_1/fakes.py
  M 240:4 FakeHTTPClient.post_servers_1234_action - F
 
  It ranks the complexity from A (best) upwards, the command above (-nd)
  says only show D or worse.
  If you look at these methods they are getting out of hand and are
  becoming difficult to understand.
  I like the idea of having a threshold that says we are not going to just
  keep adding to the complexity
  of these methods.
 
  This can be enforced with:
  xenon --max-absolute E heat
  ERROR:xenon:block heat/tests/v1_1/fakes.py:240 post_servers_1234_action  
  has a rank of F
 
  [1] https://pypi.python.org/pypi/radon
  [2] https://pypi.python.org/pypi/xenon
 
  If people are open to this, I'd like to add these to the test-requirements
  and trial this in Heat
  (as part of the pep8 tox target).
 
  
 I think the idea of gating on complexity is a great idea and would like to
 see nova adopt this as well. But why not just use flake8's built in stuff?
  
  
 
  Regards
  Angus
 
  ___
  OpenStack-dev mailing list
  OpenStack-dev@lists.openstack.org
  http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
 
 
 ___
 OpenStack-dev mailing list
 OpenStack-dev@lists.openstack.org
 http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
  


___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


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 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

 It seems to function about the same as radon + xenon, where a D in that tool
 is about a --max-complexity of 22. And flake8 obeys our existing tox
 directives (such as ignoring openstack/common).

 +1 for enabling complexity checking though.

 On Thu, Oct 16, 2014 at 10: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?

 radon is the underlying reporting tool and xenon is a monitor - meaning
 it will fail if a threshold is reached.

 To save you the time:
 radon cc -nd heat
 heat/engine/stack.py
 M 809:4 Stack.delete - E
 M 701:4 Stack.update_task - D
 heat/engine/resources/server.py
 M 738:4 Server.handle_update - D
 M 891:4 Server.validate - D
 heat/openstack/common/jsonutils.py
 F 71:0 to_primitive - D
 heat/openstack/common/config/generator.py
 F 252:0 _print_opt - D
 heat/tests/v1_1/fakes.py
 M 240:4 FakeHTTPClient.post_servers_1234_action - F

 It ranks the complexity from A (best) upwards, the command above (-nd)
 says only show D or worse.
 If you look at these methods they are getting out of hand and are
 becoming difficult to understand.
 I like the idea of having a threshold that says we are not going to just
 keep adding to the complexity
 of these methods.

 This can be enforced with:
 xenon --max-absolute E heat
 ERROR:xenon:block heat/tests/v1_1/fakes.py:240 post_servers_1234_action
 has a rank of F

 [1] https://pypi.python.org/pypi/radon
 [2] https://pypi.python.org/pypi/xenon

 If people are open to this, I'd like to add these to the test-requirements
 and trial this in Heat
 (as part of the pep8 tox target).

 Regards
 Angus

 ___
 OpenStack-dev mailing list
 OpenStack-dev@lists.openstack.org
 http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev



 ___
 OpenStack-dev mailing list
 OpenStack-dev@lists.openstack.org
 http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev




-- 
Rackspace Australia

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


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 experience with these or other tools?



 Flake8 (and thus hacking) has built in McCabe Complexity checking.

 flake8 --select=C --max-complexity 10

 https://github.com/flintwork/mccabe
 http://flake8.readthedocs.org/en/latest/warnings.html

 Example on heat: http://paste.openstack.org/show/121561
 Example in nova (max complexity of 20):
 http://paste.openstack.org/show/121562



Cool, no need to add to requirements:-) Thanks, I didn't think to look at
flake8.



 radon is the underlying reporting tool and xenon is a monitor - meaning
 it will fail if a threshold is reached.

 To save you the time:
 radon cc -nd heat
 heat/engine/stack.py
 M 809:4 Stack.delete - E
 M 701:4 Stack.update_task - D
 heat/engine/resources/server.py
 M 738:4 Server.handle_update - D
 M 891:4 Server.validate - D
 heat/openstack/common/jsonutils.py
 F 71:0 to_primitive - D
 heat/openstack/common/config/generator.py
 F 252:0 _print_opt - D
 heat/tests/v1_1/fakes.py
 M 240:4 FakeHTTPClient.post_servers_1234_action - F

 It ranks the complexity from A (best) upwards, the command above (-nd)
 says only show D or worse.
 If you look at these methods they are getting out of hand and are
 becoming difficult to understand.
 I like the idea of having a threshold that says we are not going to just
 keep adding to the complexity
 of these methods.

 This can be enforced with:
 xenon --max-absolute E heat
 ERROR:xenon:block heat/tests/v1_1/fakes.py:240 post_servers_1234_action
 has a rank of F

 [1] https://pypi.python.org/pypi/radon
 [2] https://pypi.python.org/pypi/xenon

 If people are open to this, I'd like to add these to the
 test-requirements and trial this in Heat
 (as part of the pep8 tox target).


 I think the idea of gating on complexity is a great idea and would like to
 see nova adopt this as well. But why not just use flake8's built in stuff?



Totally, it's the end result I want, I not am not worried about which tool
we use.

-Angus



 Regards
 Angus

 ___
 OpenStack-dev mailing list
 OpenStack-dev@lists.openstack.org
 http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev



 ___
 OpenStack-dev mailing list
 OpenStack-dev@lists.openstack.org
 http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


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
'LibvirtDriver._get_guest_config' is too complex (67)

http://git.openstack.org/cgit/openstack/nova/tree/nova/virt/libvirt/driver.py#n373
http://git.openstack.org/cgit/openstack/nova/tree/nova/virt/libvirt/driver.py#n3736

to

*http://git.openstack.org/cgit/openstack/nova/tree/nova/virt/libvirt/driver.py#n4113
http://git.openstack.org/cgit/openstack/nova/tree/nova/virt/libvirt/driver.py#n4113*




First step in fixing this, put a cap on it:  goog_106984861
https://review.openstack.org/129125




 +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 Salkeld
  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 McCabe Complexity checking.
 
  flake8 --select=C --max-complexity 10
 
  https://github.com/flintwork/mccabe
  http://flake8.readthedocs.org/en/latest/warnings.html
 
  Example on heat: http://paste.openstack.org/show/121561
  Example in nova (max complexity of 20):
  http://paste.openstack.org/show/121562
 
 
  
   radon is the underlying reporting tool and xenon is a monitor -
 meaning
   it will fail if a threshold is reached.
  
   To save you the time:
   radon cc -nd heat
   heat/engine/stack.py
   M 809:4 Stack.delete - E
   M 701:4 Stack.update_task - D
   heat/engine/resources/server.py
   M 738:4 Server.handle_update - D
   M 891:4 Server.validate - D
   heat/openstack/common/jsonutils.py
   F 71:0 to_primitive - D
   heat/openstack/common/config/generator.py
   F 252:0 _print_opt - D
   heat/tests/v1_1/fakes.py
   M 240:4 FakeHTTPClient.post_servers_1234_action - F
  
   It ranks the complexity from A (best) upwards, the command above (-nd)
   says only show D or worse.
   If you look at these methods they are getting out of hand and are
   becoming difficult to understand.
   I like the idea of having a threshold that says we are not going to
 just
   keep adding to the complexity
   of these methods.
  
   This can be enforced with:
   xenon --max-absolute E heat
   ERROR:xenon:block heat/tests/v1_1/fakes.py:240
 post_servers_1234_action
   has a rank of F
  
   [1] https://pypi.python.org/pypi/radon
   [2] https://pypi.python.org/pypi/xenon
  
   If people are open to this, I'd like to add these to the
 test-requirements
   and trial this in Heat
   (as part of the pep8 tox target).
  
 
  I think the idea of gating on complexity is a great idea and would like
 to
  see nova adopt this as well. But why not just use flake8's built in
 stuff?
 
 
  
   Regards
   Angus
  
   ___
   OpenStack-dev mailing list
   OpenStack-dev@lists.openstack.org
   http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
  
  
  ___
  OpenStack-dev mailing list
  OpenStack-dev@lists.openstack.org
  http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
 


___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


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 maintainable.


 Well this is scary:

 ./nova/virt/libvirt/driver.py:3736:1: C901 'LibvirtDriver._get_guest_config' 
 is too complex (67)

 http://git.openstack.org/cgit/openstack/nova/tree/nova/virt/libvirt/driver.py#n373
  
 http://git.openstack.org/cgit/openstack/nova/tree/nova/virt/libvirt/driver.py#n3736

 to

 *http://git.openstack.org/cgit/openstack/nova/tree/nova/virt/libvirt/driver.py#n4113
  
 http://git.openstack.org/cgit/openstack/nova/tree/nova/virt/libvirt/driver.py#n4113*




 First step in fixing this, put a cap on it:  http://goog_106984861
 https://review.openstack.org/129125




 +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 Salkeld
  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 McCabe Complexity checking.
 
  flake8 --select=C --max-complexity 10
 
  https://github.com/flintwork/mccabe
  http://flake8.readthedocs.org/en/latest/warnings.html
 
  Example on heat: http://paste.openstack.org/show/121561
  Example in nova (max complexity of 20):
  http://paste.openstack.org/show/121562
 
 
  
   radon is the underlying reporting tool and xenon is a monitor -
 meaning
   it will fail if a threshold is reached.
  
   To save you the time:
   radon cc -nd heat
   heat/engine/stack.py
   M 809:4 Stack.delete - E
   M 701:4 Stack.update_task - D
   heat/engine/resources/server.py
   M 738:4 Server.handle_update - D
   M 891:4 Server.validate - D
   heat/openstack/common/jsonutils.py
   F 71:0 to_primitive - D
   heat/openstack/common/config/generator.py
   F 252:0 _print_opt - D
   heat/tests/v1_1/fakes.py
   M 240:4 FakeHTTPClient.post_servers_1234_action - F
  
   It ranks the complexity from A (best) upwards, the command above (-nd)
   says only show D or worse.
   If you look at these methods they are getting out of hand and are
   becoming difficult to understand.
   I like the idea of having a threshold that says we are not going to
 just
   keep adding to the complexity
   of these methods.
  
   This can be enforced with:
   xenon --max-absolute E heat
   ERROR:xenon:block heat/tests/v1_1/fakes.py:240
 post_servers_1234_action
   has a rank of F
  
   [1] https://pypi.python.org/pypi/radon
   [2] https://pypi.python.org/pypi/xenon
  
   If people are open to this, I'd like to add these to the
 test-requirements
   and trial this in Heat
   (as part of the pep8 tox target).
  
 
  I think the idea of gating on complexity is a great idea and would like
 to
  see nova adopt this as well. But why not just use flake8's built in
 stuff?
 
 
  
   Regards
   Angus
  
   ___
   OpenStack-dev mailing list
   OpenStack-dev@lists.openstack.org
   http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
  
  
  ___
  OpenStack-dev mailing list
  OpenStack-dev@lists.openstack.org
  http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
 



 ___
 OpenStack-dev mailing list
 OpenStack-dev@lists.openstack.org
 http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

 Wow!!  Great topic, and a lot of opportunities in Cinder for the Kilo
release.  Thanks!!
___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


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 maintainable.


 Well this is scary:

 ./nova/virt/libvirt/driver.py:3736:1: C901 'LibvirtDriver._get_guest_config' 
 is too complex (67)

 http://git.openstack.org/cgit/openstack/nova/tree/nova/virt/libvirt/driver.py#n373
  
 http://git.openstack.org/cgit/openstack/nova/tree/nova/virt/libvirt/driver.py#n3736

 to

 *http://git.openstack.org/cgit/openstack/nova/tree/nova/virt/libvirt/driver.py#n4113
  
 http://git.openstack.org/cgit/openstack/nova/tree/nova/virt/libvirt/driver.py#n4113*




 First step in fixing this, put a cap on it:  http://goog_106984861
 https://review.openstack.org/129125


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.

-Angus





 +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 Salkeld
  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 McCabe Complexity checking.
 
  flake8 --select=C --max-complexity 10
 
  https://github.com/flintwork/mccabe
  http://flake8.readthedocs.org/en/latest/warnings.html
 
  Example on heat: http://paste.openstack.org/show/121561
  Example in nova (max complexity of 20):
  http://paste.openstack.org/show/121562
 
 
  
   radon is the underlying reporting tool and xenon is a monitor -
 meaning
   it will fail if a threshold is reached.
  
   To save you the time:
   radon cc -nd heat
   heat/engine/stack.py
   M 809:4 Stack.delete - E
   M 701:4 Stack.update_task - D
   heat/engine/resources/server.py
   M 738:4 Server.handle_update - D
   M 891:4 Server.validate - D
   heat/openstack/common/jsonutils.py
   F 71:0 to_primitive - D
   heat/openstack/common/config/generator.py
   F 252:0 _print_opt - D
   heat/tests/v1_1/fakes.py
   M 240:4 FakeHTTPClient.post_servers_1234_action - F
  
   It ranks the complexity from A (best) upwards, the command above (-nd)
   says only show D or worse.
   If you look at these methods they are getting out of hand and are
   becoming difficult to understand.
   I like the idea of having a threshold that says we are not going to
 just
   keep adding to the complexity
   of these methods.
  
   This can be enforced with:
   xenon --max-absolute E heat
   ERROR:xenon:block heat/tests/v1_1/fakes.py:240
 post_servers_1234_action
   has a rank of F
  
   [1] https://pypi.python.org/pypi/radon
   [2] https://pypi.python.org/pypi/xenon
  
   If people are open to this, I'd like to add these to the
 test-requirements
   and trial this in Heat
   (as part of the pep8 tox target).
  
 
  I think the idea of gating on complexity is a great idea and would like
 to
  see nova adopt this as well. But why not just use flake8's built in
 stuff?
 
 
  
   Regards
   Angus
  
   ___
   OpenStack-dev mailing list
   OpenStack-dev@lists.openstack.org
   http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
  
  
  ___
  OpenStack-dev mailing list
  OpenStack-dev@lists.openstack.org
  http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
 



 ___
 OpenStack-dev mailing list
 OpenStack-dev@lists.openstack.org
 http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


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:

(pep8)steven@undermined:~/openstack/openstack/heat% flake8
--max-complexity 0 | grep 'is too complex' | sort -k 2 -rn -t '(' | head
./heat/engine/stack.py:809:1: C901 'Stack.delete' is too complex (28)
./heat/tests/v1_1/fakes.py:240:1: C901
'FakeHTTPClient.post_servers_1234_action' is too complex (24)
./heat/engine/resources/server.py:738:1: C901 'Server.handle_update' is
too complex (23)
./doc/source/conf.py:62:1: C901 'write_autodoc_index' is too complex (18)
./heat/engine/stack.py:701:1: C901 'Stack.update_task' is too complex (17)
./heat/engine/resources/instance.py:709:1: C901 'Instance.handle_update'
is too complex (17)
./heat/engine/rsrc_defn.py:340:1: C901 'ResourceDefinition.__getitem__'
is too complex (16)
./heat/engine/resources/random_string.py:175:1: C901
'RandomString._generate_random_string' is too complex (16)
./heat/api/cfn/v1/stacks.py:277:1: C901
'StackController.create_or_update' is too complex (16)
./heat/engine/watchrule.py:358:1: C901 'rule_can_use_sample' is too
complex (15)

Cheers,
-- 
Steve
Oh, in case you got covered in that Repulsion Gel, here's some advice
the lab boys gave me: [paper rustling] DO NOT get covered in the
Repulsion Gel.
 - Cave Johnson, CEO of Aperture Science

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


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, 
but the number of methods gets larger.  A large number of methods is 
itself a form of complexity.  It is not clear to me that said re-org has 
necessarily made the class easier to understand.  I can also break one 
class into two, but it is not clear to me that the project has necessarily 
become easier to understand.  While it is true that when you truly make a 
project easier to understand you sometimes break it into more classes, it 
is also true that you can do a bad job of re-organizing a set of classes 
while still reducing the size of the largest method.  Has the McCabe 
metric been evaluated on Python projects?  There is a danger in focusing 
on what is easy to measure if that is not really what you want to 
optimize.

BTW, I find that one of the complexity issues for me when I am learning 
about a Python class is doing the whole-program type inference so that I 
know what the arguments are.  It seems to me that if you want to measure 
complexity of Python code then something like the complexity of the 
argument typing should be taken into account.

Regards,
Mike___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


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 methods that
do one thing well (simple and easy to read/test by it's self). But it stops
complex methods growing out of hand, by just failing the test and now you
have to look at refactoring. I think reviews can cover the how good is
your refactor.


  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, but the
 number of methods gets larger.  A large number of methods is itself a form
 of complexity.  It is not clear to me that said re-org has necessarily made
 the class easier to understand.  I can also break one class into two, but
 it is not clear to me that the project has necessarily become easier to
 understand.


No, but a well written class *should* be easy to understand in isolation. I
don't think there is a tool to say 'your code sucks'.

-A


  While it is true that when you truly make a project easier to understand
 you sometimes break it into more classes, it is also true that you can do a
 bad job of re-organizing a set of classes while still reducing the size of
 the largest method.  Has the McCabe metric been evaluated on Python
 projects?  There is a danger in focusing on what is easy to measure if that
 is not really what you want to optimize.

 BTW, I find that one of the complexity issues for me when I am learning
 about a Python class is doing the whole-program type inference so that I
 know what the arguments are.  It seems to me that if you want to measure
 complexity of Python code then something like the complexity of the
 argument typing should be taken into account.

 Regards,
 Mike
 ___
 OpenStack-dev mailing list
 OpenStack-dev@lists.openstack.org
 http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


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   mich...@the-davies.net
Rackspace Australia
___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev