[mccabe] Improve example (C901)#25287
Conversation
The original `if a/if b/if c` ladder and the early-return rewrite both have McCabe complexity 4, so the docs were telling readers to "use instead" code that the rule still flags identically. @MichaReiser suggested (in astral-sh#25028) either replacing the example with one that actually reduces complexity, or removing the "Use instead" section entirely. Picking the second option keeps the docs honest without committing to one specific refactoring style. Adds a short paragraph pointing to the real techniques that reduce McCabe complexity (extracting helpers, lookup tables, lower branching factor) and calls out the common misconception that flipping conditions into guard clauses lowers the complexity on its own. Closes astral-sh#25028.
|
| /// branching factor than the original. Note that mechanically inverting | ||
| /// conditions to use guard clauses does not, on its own, lower the McCabe | ||
| /// complexity, since the number of decision points is unchanged. |
There was a problem hiding this comment.
This second sentence only makes sense in the context of the now-deleted code example. I honestly don't really find this paragraph to be an improvement at all because it basically says "To reduce a function's complexity, reduce its complexity" but much more verbosely.
My preference would either be an example like I left on #25186 or just to delete this paragraph and the code example entirely.
The change here is also missing this aspect of my suggestion:
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.
There was a problem hiding this comment.
Done in eefc9bb. Pulled in the dict-lookup Before/After from your #25186 comment and added the max-complexity note.
The first example's McCabe complexity is 6 (one for the function plus one per if), so the note says "assume a lint.mccabe.max-complexity of 5 or less, ... The default is 10, so neither example triggers C901 out of the box." Kept the single sentence about guard-clause inversion not lowering complexity since that is still the original motivation for #25028.
mccabe] Drop misleading "Use instead" example from C901 docsmccabe] Drop misleading "Use instead" example from C901 docs
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).
Merging this PR will not alter performance
Comparing 
Footnotes
|
mccabe] Drop misleading "Use instead" example from C901 docsmccabe] Improve example (C901)
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>
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>
The C901 docstring currently pairs a 3-deep
if/elseexample 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-allanduvx prek run --from-ref mainboth pass.closes #25028