[pylint] - Implement too-many-attributes rule (PLR0902)
#9844
Conversation
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| PLR0902 | 806 | 806 | 0 | 0 | 0 |
There was a problem hiding this comment.
Thanks for contributing this rule.
Code: There are two cases where the rule currently misses field declarations that should be supported.
An open question for us as maintainers is whether we want to merge the rule today. The problem we're facing is that many users use --select ALL and consider it as Ruff's "recommended" rule set. I see that this rule is useful when used in a mindful way, but it could guide less aware users in the direction that having classes with many attributes is always bad and is generally considered bad practice.
We hope to solve the ALL problem by recategorizing our rules so that more opinionated rules require explicit opt-in. But we may need to wait to add new rules till then.
crates/ruff_linter/src/rules/pylint/rules/too_many_attributes.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/pylint/rules/too_many_attributes.rs
Outdated
Show resolved
Hide resolved
This lint is current in the experimental state, so it won't be enabled by default ...? |
That's correct. But the intent would be to stabilize the rule eventually (ideally one or two minor releases later). |
Understood. So this pull request would be put on the backburner and wait until a better grouping mechanism is added? |
|
I discussed this internally and @charliermarsh is fine with adding new pylint rules, even if they are very opinionated. |
There was a problem hiding this comment.
This looks good to me but I want that @charliermarsh takes a look because I wonder if we extract the variables already as part of the semantic model ( I tried scope.bindings but this doesn't give me all fields).
| # max of 7 attributes by default, can be configured | ||
| self.attr0: int = 0 | ||
| self.attr0 = 1 | ||
| self.attr1, self.attr2 = 1, 2 |
There was a problem hiding this comment.
Nice catch. I wasn't aware that this is allowed too
crates/ruff_linter/src/rules/pylint/rules/too_many_attributes.rs
Outdated
Show resolved
Hide resolved
| self.secondary_worm_color = "Whitish" | ||
| # typed assignments | ||
| self.a_string: str = "String" | ||
| self.a_number: float = 3.1415926 |
There was a problem hiding this comment.
Can we add a few more tests:
- a test for a class that has exactly 7 attributes
- a test for a class that assigns to attributes in method other than
__init__ - a test for a class that has fewer than 7 attributes
- a test for a class that assigns the same variable multiple times (e.g. defines the attributes and assigns in
__init__
... attributes declared outside class methods
|
Comments addressed and rebased against the latest main branch. |
|
I think this is the same as #10431. We need to decide which of the two PRs we want to move forward. |
I hadn't noticed this PR existed before making my own, I concede to this one! |
|
Thank you for contributing to this new rule, and I am sorry that it took so long to decide. We agree that the rule itself is useful, and we're open to merging the rule once #1774 is complete, but we don't want to merge the rule today. The reasons are:
I'm sorry that it took us so long to come to that conclusion and that we haven't flagged this on the pylint issue. I plan to go through the Pylint rules and mark the rules that are unlikely to be accepted today, but I first have to catch up on open linter PRs. Thank you again for contributing and I hope we can merge this rule in the future, once we figured out the right place for it. |
Summary
Adds too-many-attributes rule (PLR0902) from Pylint. This implementation has a similar limitation as the Pylint one, where dynamically generated class attributes (e.g. through
setattroreval) are not considered.Test Plan
A new test has been added and can be executed using
cargo test.