docs: clarify C901 complexity example#25186
Closed
ya-nsh wants to merge 1 commit into
Closed
Conversation
ntBre
reviewed
May 15, 2026
ntBre
left a comment
Contributor
There was a problem hiding this comment.
Thanks! I think this is a little better, but it feels a bit outside the spirit of the rule to me just to move the complexity into a helper function (which would actually make reading/reviewing the code more complex, in my opinion). Codex suggested something like the following, where we replace a sequence of if conditions with a dict lookup:
# Before: branch-heavy lookup
def normalize_status(status):
if status == "new":
return "queued"
if status == "queued":
return "running"
if status == "running":
return "done"
if status == "failed":
return "retry"
if status == "cancelled":
return "closed"
return "unknown"
# After: the decision table is data, not control flow
STATUS_TRANSITIONS = {
"new": "queued",
"queued": "running",
"running": "done",
"failed": "retry",
"cancelled": "closed",
}
def normalize_status_lookup(status):
return STATUS_TRANSITIONS.get(status, "unknown")That specifically is pretty verbose, but I kind of like something in that direction.
I also realized that the default value of max-complexity is 10, so neither the existing nor new input examples actually trigger C901 out of the box. I don't think we need to extend the length of the examples to meet that level of complexity, but we should probably note that somewhere (e.g. "These examples assume a lint.mccabe.max-complexity of ..."). I'll try to remember to comment on #18972 to mark this rule as an exception, assuming we go that route.
This comment was marked as low quality.
This comment was marked as low quality.
adityasingh2400
added a commit
to adityasingh2400/ruff
that referenced
this pull request
May 21, 2026
ntBre's comment on the prior revision was right that the techniques paragraph just paraphrased "to reduce complexity, reduce complexity" without showing what an actual reduction looks like. Replaced the nested if/else example with the Before/After dict-lookup pair from your suggestion on astral-sh#25186, restoring the Use instead structure with an example that actually moves decision points out of control flow and into data. The Before has 5 top-level if statements so the function's McCabe complexity is 6 (one for the function plus one per if). The After is 1, so the rewrite is a real reduction, not a wash. Added a note that both examples sit under the default lint.mccabe.max-complexity of 10 and would need max-complexity = 5 or less to actually trip C901. Kept the single sentence about guard clauses not lowering complexity on their own, since that is still the load-bearing point for the issue this PR closes (astral-sh#25028).
ntBre
added a commit
that referenced
this pull request
May 21, 2026
The C901 docstring currently pairs a 3-deep `if/else` example with a guard-clause rewrite under "Use instead". Both versions have McCabe complexity 4, so the rewrite trips (or passes) C901 identically and the docs end up teaching the false equivalence "guard clauses lower McCabe complexity." A reader filed #25028 to flag this. In that thread, MichaReiser said the docs should either use an example that actually reduces complexity or drop the "Use instead" block altogether. A prior attempt (#25137) was closed with the same comment, and a follow-up trying a lookup-table example (#25186) was also closed, which makes me think the safer option is the second one MichaReiser offered. I dropped the "Use instead" block and added a short paragraph naming the techniques that genuinely reduce McCabe complexity (extracting helpers, lookup tables, lower branching factor), with an explicit note that mechanical guard-clause inversion does not. Docstring-only. `cargo dev generate-all` and `uvx prek run --from-ref main` both pass. closes #25028 --------- Co-authored-by: Brent Westbrook <brentrwestbrook@gmail.com>
thejchap
pushed a commit
to thejchap/ruff
that referenced
this pull request
May 23, 2026
The C901 docstring currently pairs a 3-deep `if/else` example with a guard-clause rewrite under "Use instead". Both versions have McCabe complexity 4, so the rewrite trips (or passes) C901 identically and the docs end up teaching the false equivalence "guard clauses lower McCabe complexity." A reader filed astral-sh#25028 to flag this. In that thread, MichaReiser said the docs should either use an example that actually reduces complexity or drop the "Use instead" block altogether. A prior attempt (astral-sh#25137) was closed with the same comment, and a follow-up trying a lookup-table example (astral-sh#25186) was also closed, which makes me think the safer option is the second one MichaReiser offered. I dropped the "Use instead" block and added a short paragraph naming the techniques that genuinely reduce McCabe complexity (extracting helpers, lookup tables, lower branching factor), with an explicit note that mechanical guard-clause inversion does not. Docstring-only. `cargo dev generate-all` and `uvx prek run --from-ref main` both pass. closes astral-sh#25028 --------- Co-authored-by: Brent Westbrook <brentrwestbrook@gmail.com>
anishgirianish
pushed a commit
to anishgirianish/ruff
that referenced
this pull request
May 28, 2026
The C901 docstring currently pairs a 3-deep `if/else` example with a guard-clause rewrite under "Use instead". Both versions have McCabe complexity 4, so the rewrite trips (or passes) C901 identically and the docs end up teaching the false equivalence "guard clauses lower McCabe complexity." A reader filed astral-sh#25028 to flag this. In that thread, MichaReiser said the docs should either use an example that actually reduces complexity or drop the "Use instead" block altogether. A prior attempt (astral-sh#25137) was closed with the same comment, and a follow-up trying a lookup-table example (astral-sh#25186) was also closed, which makes me think the safer option is the second one MichaReiser offered. I dropped the "Use instead" block and added a short paragraph naming the techniques that genuinely reduce McCabe complexity (extracting helpers, lookup tables, lower branching factor), with an explicit note that mechanical guard-clause inversion does not. Docstring-only. `cargo dev generate-all` and `uvx prek run --from-ref main` both pass. closes astral-sh#25028 --------- Co-authored-by: Brent Westbrook <brentrwestbrook@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #25028.
Validation
git diff --checkNote: I attempted the repository preflight command
uvx prek run -a, but this environment does not have the Rust toolchain installed, so therustfmthook failed before running:This change only updates Rust doc comments for the generated rule documentation.