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

PLR0912: Pylint counts try: ... except: statements as single branch #11205

Closed
Kakadus opened this issue Apr 29, 2024 · 5 comments · Fixed by #11487
Closed

PLR0912: Pylint counts try: ... except: statements as single branch #11205

Kakadus opened this issue Apr 29, 2024 · 5 comments · Fixed by #11487
Assignees
Labels
rule Implementing or modifying a lint rule

Comments

@Kakadus
Copy link

Kakadus commented Apr 29, 2024

PLR0912 is not equivalent to pylint. The following function is accepted by pylint but rejected by ruff:

def capital(country):
    if country == "Australia":
        return "Canberra"
    elif country == "Brazil":
        return "Brasilia"
    elif country == "Canada":
        return "Ottawa"
    elif country == "England":
        return "London"
    elif country == "France":
        return "Paris"
    elif country == "Germany":
        return "Berlin"
    elif country == "Poland":
        return "Warsaw"
    elif country == "Romania":
        return "Bucharest"
    elif country == "Spain":
        return "Madrid"
    elif country == "Thailand":
        return "Bangkok"
    elif country == "Turkey":
        return "Ankara"
    try:
        return None
    except:
        return "Some"

Pylint only detects 12 branches, while ruff counts 13. Is this a bug or a feature? If the latter, it should probably be documented. I can also imagine a setting, enabling/disabling this behavior

@MichaReiser
Copy link
Member

The rule was added in #2550 but there's no conversation in that PR nor is there any change to the rule explaining if that divergence is indeed intentional.

@chanman3388 as author of the rule. What's your take on this?

To me, counting except as its own branch does make sense. Because the except might or might not be executed. So the try block has two possible exit branches.

@dhruvmanila dhruvmanila added the rule Implementing or modifying a lint rule label Apr 30, 2024
@chanman3388
Copy link
Contributor

The rule was added in #2550 but there's no conversation in that PR nor is there any change to the rule explaining if that divergence is indeed intentional.

@chanman3388 as author of the rule. What's your take on this?

To me, counting except as its own branch does make sense. Because the except might or might not be executed. So the try block has two possible exit branches.

I don't believe that this divergence was intentional, pylint does some odd things, I'd be inclined to agree with @MichaReiser that is is its own branch or brances, but I guess the question to me is whether we should remain faithful to pylint's implementation which looks like:

    def visit_try(self, node: nodes.Try) -> None:
        """Increments the branches counter."""
        branches = len(node.handlers)
        if node.orelse:
            branches += 1
        if node.finalbody:
            branches += 1
        self._inc_branch(node, branches)
        self._inc_all_stmts(branches)

or if we should go with our own interpretation. Looking at the python AST I can't tell if this was oversight or if the intention was to consider exceptions differently?

@charliermarsh
Copy link
Member

It looks like the difference is whether the try body counts as a branch? I'd say it should, probably. Although it's perhaps strange that finally counts as a branch, since it always runs? Ultimately, though, the rule is trying to capture some measure of complexity, and both the try body and the finally contribute to complexity in a way...

@charliermarsh
Copy link
Member

I'd be fine to remove the body from the count though. It's a bit more consistent with Pylint and is pretty marginal either way.

@dhruvmanila
Copy link
Member

Related: #11421

charliermarsh added a commit that referenced this issue May 22, 2024
## Summary

Matching Pylint, we now omit the `try` body itself from branch counting.
Each `except` counts as a branch, as does the `else` and the `finally`.

Closes #11205.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants