ci(ruff): Enforce the default C901 complexity#3531
Conversation
https://docs.astral.sh/ruff/settings/#lint_mccabe_max-complexity Previously 18, the default is 10. As this rule was not in `[tool.ruff.lint.select]`, it was not used. Setting such a high limit hides opportunities to simplify large, complex functions. Many cases were resolved/improved in vega#3431. The remainder can use the *# noqa: C901* comment to serve as a TODO
These are mostly too complex to handle in this PR, it would be a very difficult review
Ideally tests would be split up enough to not fail this lint, but not a major issue
|
|
||
| @traitlets.observe("chart") | ||
| def _on_change_chart(self, change): | ||
| def _on_change_chart(self, change): # noqa: C901 |
There was a problem hiding this comment.
I think this was the highest, at around 20
|
|
||
| def _check_if_can_be_layered(spec: LayerType | dict[str, Any]) -> None: | ||
| # C901 fixed in https://github.com/vega/altair/pull/3520 | ||
| def _check_if_can_be_layered(spec: LayerType | dict[str, Any]) -> None: # noqa: C901 |
|
Thanks for working through these rules @dangotbanned! I'll defer to others, but I don't personally find C901 all that helpful. We should of course use functions and classes to minimize duplication of logic, but for an inherently complex function that doesn't contain logic that's useful elsewhere, I don't personally get much benefit from it being broken down into smaller functions. And I suppose this is reflected in my coding style 😄 |
Totally valid point @jonmmease! Appreciate your thoughts. Choosing to use Personally, the benefit to me when writing new code is that the warning prompts me to consider if there is an alternative way to write the function:
If there isn't, or if it is time sensitive (or I disagree with the warning) - add a # noqa: C901 and move on. |
|
Yeah, it's easy to disable when needed. So no objection, and the small changes you needed to make to pass the check all look positive to me. |
jonmmease
left a comment
jonmmease
left a comment
There was a problem hiding this comment.
Looks good. Feel free to merge when you're ready!
Thanks @jonmmease! FYI if at any time you find it frustrating to add a lot of ignore comments (for a single file), you can always add to our tool.ruff.lint.per-file-ignores table instead |
3 participants
References
Previously was set at 18, the default is 10.
As the rule was not in
[tool.ruff.lint.select], the setting was never used.Setting such a high limit hides opportunities to simplify large, complex functions.
Many cases were resolved/improved in #3431.
There were 2 I fixed in this PR as they were quite simple changes.
The remainder use the # noqa: C901 comment to serve as a TODO.