Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make the mccabe checker ignore the case of a single top-level elif chain or match statement #3597

Open
saolof opened this issue Mar 18, 2023 · 5 comments
Labels
needs-decision Awaiting a decision from a maintainer rule Implementing or modifying a lint rule

Comments

@saolof
Copy link

saolof commented Mar 18, 2023

This is a somewhat common edge case where there shouldn't be a need to use noqa comments. Non-looping functions with a nesting depth of 1 should be excluded from the mccabe checker, or there should be a separate error code for this particular kind of pattern if you want to lint or ignore it separately, such as "elif chain exceeds mccabe complexity".

Incidentally, while discussing code complexity KPIs, being able to set a maximum allowed nesting depth would be nice.

@antonagestam
Copy link

I'd like to work on this.

@charliermarsh
Copy link
Member

Confirming my understanding: are you describing something akin to a top-level switch statement? Like:

if foo == "bar":
  return 1
elif foo == "baz:
  return 2:
else ...

@charliermarsh charliermarsh added the question Asking for support or clarification label Mar 19, 2023
@saolof
Copy link
Author

saolof commented Mar 19, 2023

Yes, exactly. There is a clear difference in complexity between a match statement with 10 cases, and a piece of code that has 3 nested for loops with if-clauses randomly distributed inside.

In the case where you are doing the switch based on type you can of course split it up with the functools.dispatch decorator or methods, and in the case where you just return a value you can use a dictionary, but apart from those cases, one big match with any nested behaviour split off into a separate function is usually simpler than any replacement.

@antonagestam
Copy link

@saolof I agree, and I think it can sometimes be detrimental to try and break apart code like this. The result isn't likely to be easier for a human to reason about, and is likely to lower coherence. Single-level structures are the exception to the rule, because they are already easy to reason about and follow.

@antonagestam
Copy link

Sorry, I haven't had much time to spend on this. I might get to it at some point, but if someone else is keen to work on this, please go ahead.

@charliermarsh charliermarsh added rule Implementing or modifying a lint rule needs-decision Awaiting a decision from a maintainer and removed question Asking for support or clarification labels Jul 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-decision Awaiting a decision from a maintainer rule Implementing or modifying a lint rule
Projects
None yet
Development

No branches or pull requests

3 participants