def inside def

Question

def inside def

evvers opened this issue 10 years ago · 14 comments

Hi!

Why def inside def increase complexity of outer function?
def inside def looks like method inside class. Maybe, complexity of inner function should be independent from outer function?

For example, complexity of test_nested_functions_snippet equals to 1 instead of 3.

Answer 1 · 2014-05-15T22:51:52.000Z

i.e. complexity of all functions from "nested" test equals to 1.

Answer 2 · 2014-05-16T21:10:03.000Z

I appreciate your pull request but I'm not sure I agree. The algorithm doesn't explicitly cover this case in the documentation I've seen. I'll have to think about this.

Answer 3 · 2014-05-16T22:19:47.000Z

Thank you for answer.
One example.
Users of git-pre-commit-hook send me feedback
They wrote that functions like this:

def func():
    def helper_1():
       ...
    def helper_n():
       ....
    initialization
    actions

doesn't passed complexity check
But

class SomeClass(object):
    def __init__(self):
        initialization()
    def helper_1(self):
        ....
    def helper_n(self):
        ....
    def __call__(self):
        actions

passed complexity check.
For my opinion, this is inconsistent behavior.

Answer 4 · 2014-05-16T22:56:43.000Z

I don't agree it is inconsistent behaviour. The complexity checker, in part, is meant to help you understand when your code might be too difficult for someone to understand. I would find those nested functions far more baffling (especially given how uncommon it is) than a class with methods.

Answer 5 · 2014-05-16T23:44:52.000Z

I agree.
They develop very specific soft:)
But class looks ugly too:)
Because, this case is very rare most users doesn't notice changes at checker at all

Answer 6 · 2014-05-17T00:19:00.000Z

The difference is that even though it is rare, I think the existing behaviour is correct. Changing it to allow for users with bad habits is not what I'm interested in. Classes may look ugly but that doesn't increase their difficulty to understand the code.

Answer 7 · 2014-05-17T01:07:14.000Z

Hmm, I think you're right.
Close it:)

Answer 8 · 2015-11-08T11:24:20.000Z

You might want to have a policy in which all class methods (private or public require an API doc) but
some frameworks (ex Twisted) want you to create multiple functions and chain them.

It would be nice, to provide at least the option to consider embedded method just like normal class method.

Answer 9 · 2015-11-08T14:40:44.000Z

It would be nice, to provide at least the option to consider embedded method just like normal class method.

It would be convenient for users, certainly. If you inspect the project though, there's no place to easily make that distinction without doing some back flips to pre-analyze the AST, and mark things with nested functions as classes instead (i.e., literally changing the AST and invalidating it).

Answer 10 · 2015-11-08T15:35:28.000Z

I did check the current mccabe.py before posting the comment and I agree the implementing this is not easy.

I think the existing behaviour is correct. Changing it to allow for users with bad habits is not what I'm interested in. Classes may look ugly but that doesn't increase their difficulty to understand the code.

From the past conversation, my understanding was that even if I would submit such a patch, it would be rejects, as it is not aligned with the goal of this project.

For me, this was just a weekend hack so I don't really mind if it is not implemented.

I just wanted to check if you still think that this is a bug or a feature :)

Answer 11 · 2015-11-08T15:39:05.000Z

So I think turning it off by default is a problem. If there was a way to efficiently pre-process the AST for this (or even to do it just in time) based on a flag (which users would understand could cause significant performance degradation) and provided it's well tested and correct, I wouldn't hesitate to accept a PR because, as you point out, within certain frameworks this is absolutely a necessity of life.

Answer 12 · 2015-11-08T15:40:23.000Z

My comment around "users with bad habits" is also more informed by certain projects that I have worked on in the last year and a half that thoroughly abuse closures in this way without needing closures in the first place. So, that was mostly motivated out of frustration with those projects.

Answer 13 · 2015-11-10T08:33:17.000Z

as a follow up note.

maybe this can be integrated into pylint as a custom checker.
I think that pylint already has support for detecting nested def and it has a few checks related to code complexity based on number of arguments.

Answer 14 · 2015-11-12T16:31:03.000Z

@adiroiban yeah, pylint might be a better place to integrate this feature