Re: [openstack-dev] [all] add cyclomatic complexity check to pep8 target
-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
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
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
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
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
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
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
Re: [openstack-dev] [all] add cyclomatic complexity check to pep8 target
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
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
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
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
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
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
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
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
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
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
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
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
