[`flake8-bugbear`] Implement `class-as-data-structure` (`B903`) by diceroll123 · Pull Request #9601 · astral-sh/ruff

diceroll123 · 2024-01-21T22:34:35Z

Summary

Adds class-as-data-structure rule (B903).

Took some creative liberty with this by allowing the class to have any decorators or base classes. There are years-old issues on pylint that don't approve of the strictness when it comes to these things.

Especially considering that dataclass is a decorator and namedtuple can be a base class. I feel ignoring those explicitly is redundant all things considered, but it's not a hill I'm willing to die on!

See: #970

Test Plan

cargo test

github-actions · 2024-01-21T22:47:35Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+40 -0 violations, +0 -0 fixes in 10 projects; 45 projects unchanged)

PlasmaPy/PlasmaPy (+4 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview

+ tests/particles/test_decorators.py:437:5: B903 Class could be dataclass or namedtuple
+ tests/particles/test_decorators.py:456:5: B903 Class could be dataclass or namedtuple
+ tests/particles/test_decorators.py:494:5: B903 Class could be dataclass or namedtuple
+ tests/particles/test_decorators_annotations.py:16:1: B903 Class could be dataclass or namedtuple

apache/airflow (+3 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview --select ALL

+ airflow/www/security_appless.py:27:1: B903 Class could be dataclass or namedtuple
+ providers/src/airflow/providers/celery/executors/celery_executor_utils.py:200:1: B903 Class could be dataclass or namedtuple
+ providers/src/airflow/providers/google/cloud/operators/dataflow.py:64:1: B903 Class could be dataclass or namedtuple

apache/superset (+1 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview --select ALL

+ superset/views/base_api.py:133:1: B903 Class could be dataclass or namedtuple

langchain-ai/langchain (+1 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview

+ libs/core/tests/unit_tests/tracers/test_langchain.py:103:5: B903 Class could be dataclass or namedtuple

pandas-dev/pandas (+1 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview

+ pandas/io/sas/sas7bdat.py:91:1: B903 Class could be dataclass or namedtuple

pypa/pip (+2 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview

+ tests/lib/__init__.py:1347:1: B903 Class could be dataclass or namedtuple
+ tests/unit/test_network_auth.py:331:5: B903 Class could be dataclass or namedtuple

python/mypy (+7 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview

+ misc/perf_checker.py:14:1: B903 Class could be dataclass or namedtuple
+ mypy/nodes.py:659:1: B903 Class could be dataclass or namedtuple
+ mypy/plugins/attrs.py:96:1: B903 Class could be dataclass or namedtuple
+ mypy/stubgen.py:182:1: B903 Class could be dataclass or namedtuple
+ mypy/stubutil.py:326:1: B903 Class could be dataclass or namedtuple
+ mypy/test/teststubtest.py:189:1: B903 Class could be dataclass or namedtuple
+ mypyc/ir/class_ir.py:453:1: B903 Class could be dataclass or namedtuple

reflex-dev/reflex (+1 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview

+ tests/units/utils/test_serializers.py:53:5: B903 Class could be dataclass or namedtuple

tiangolo/fastapi (+18 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview

+ docs_src/dependencies/tutorial002.py:11:1: B903 Class could be dataclass or namedtuple
+ docs_src/dependencies/tutorial002_an.py:12:1: B903 Class could be dataclass or namedtuple
+ docs_src/dependencies/tutorial002_an_py310.py:11:1: B903 Class could be dataclass or namedtuple
+ docs_src/dependencies/tutorial002_an_py39.py:11:1: B903 Class could be dataclass or namedtuple
+ docs_src/dependencies/tutorial002_py310.py:9:1: B903 Class could be dataclass or namedtuple
+ docs_src/dependencies/tutorial003.py:11:1: B903 Class could be dataclass or namedtuple
+ docs_src/dependencies/tutorial003_an.py:12:1: B903 Class could be dataclass or namedtuple
+ docs_src/dependencies/tutorial003_an_py310.py:11:1: B903 Class could be dataclass or namedtuple
+ docs_src/dependencies/tutorial003_an_py39.py:11:1: B903 Class could be dataclass or namedtuple
+ docs_src/dependencies/tutorial003_py310.py:9:1: B903 Class could be dataclass or namedtuple
+ docs_src/dependencies/tutorial004.py:11:1: B903 Class could be dataclass or namedtuple
+ docs_src/dependencies/tutorial004_an.py:12:1: B903 Class could be dataclass or namedtuple
+ docs_src/dependencies/tutorial004_an_py310.py:11:1: B903 Class could be dataclass or namedtuple
+ docs_src/dependencies/tutorial004_an_py39.py:11:1: B903 Class could be dataclass or namedtuple
+ docs_src/dependencies/tutorial004_py310.py:9:1: B903 Class could be dataclass or namedtuple
+ docs_src/python_types/tutorial010.py:1:1: B903 Class could be dataclass or namedtuple
+ tests/test_jsonable_encoder.py:17:1: B903 Class could be dataclass or namedtuple
+ tests/test_jsonable_encoder.py:22:1: B903 Class could be dataclass or namedtuple

zulip/zulip (+2 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview --select ALL

+ confirmation/models.py:252:1: B903 Class could be dataclass or namedtuple
+ zerver/lib/markdown/__init__.py:390:1: B903 Class could be dataclass or namedtuple

Changes by rule (1 rules affected)

code	total	+ violation	- violation	+ fix	- fix
B903	40	40	0	0	0

zanieb · 2024-01-24T16:16:49Z

Hi! Thanks for implementing this.

I'm hesitant to include this rule in Ruff. It looks like "too few methods" is used as a proxy for "this should be a dataclass" — it seems like a rule like "it only has an __init__, it should be a dataclass" would make more sense? Are there more use cases for this rule?

Separately, there's a false positive here for nested classes used for configuration — perhaps those should be excluded from the rule?

diceroll123 · 2024-01-25T01:49:26Z

Hi! Thanks for implementing this.

I'm hesitant to include this rule in Ruff. It looks like "too few methods" is used as a proxy for "this should be a dataclass" — it seems like a rule like "it only has an __init__, it should be a dataclass" would make more sense? Are there more use cases for this rule?

Separately, there's a false positive here for nested classes used for configuration — perhaps those should be excluded from the rule?

Tweaked accordingly!

Changing up the name of the rule slightly may be in order, I figure? 🤔 Please advise!

MichaReiser

I left an inline comment with a rule name suggestion. @AlexWaygood do you have ideas/opinions on a name for this rule?

crates/ruff_linter/src/rules/pylint/rules/too_few_public_methods.rs

AlexWaygood · 2024-02-26T17:29:43Z

@AlexWaygood do you have ideas/opinions on a name for this rule?

Possibly something like consider-using-dataclass could work well... but there might be other reasons you might want to consider using a dataclass, of course (such as code generation for several useful methods like __repr__, __eq__, etc.)!

I think the current name is okay, albeit a bit verbose, and there is some advantage to it having the same name as the original pylint rule

MichaReiser · 2024-02-26T17:33:01Z

Possibly something like consider-using-dataclass could work well... but there might be other reasons you might want to consider using a dataclass, of course (such as code generation for several useful methods like repr, eq, etc.)!

I like the name but it, unfortunately, doesn't fit our rule naming convention

I think the current name is okay, albeit a bit verbose, and there is some advantage to it having the same name as the original pylint rule

We can use the same rule code as pylint, but I would prefer another name that's more explicit about its motivation. Having a different name might also help to set expectations, considering that we're probably changing the semantics of the rule.

AlexWaygood · 2024-02-26T17:40:15Z

Maybe something like NonDataclassSimpleClass?

MichaReiser · 2024-02-27T17:21:12Z

This is interesting #10146 I haven't looked at how the two rules overlap but maybe there's an opportunity.

AlexWaygood · 2024-02-27T17:22:59Z

This is interesting #10146 I haven't looked at how the two rules overlap but maybe there's an opportunity.

That is interesting, indeed... it's flagging a different antipattern, but one that has the same solution

diceroll123 · 2024-03-15T04:57:26Z

Let me know if there's any action items blocking this PR, not sure what to make of these comments tbh

MichaReiser · 2024-04-05T10:22:09Z

Note for reviewing: The rule itself is a style rule but we need to find a good name that expresses the motivation to use dataclasses/named tuples instead of enforcing a number of public methods. The name should read as allow(rule-name).

But the intent of the rule itself fits into ruff as a style rule.

Rizhiy · 2024-09-09T22:43:32Z

@zanieb @MichaReiser Is there any progress on merging this? I think it would be a beneficial rule to add.

In addition to suggesting data-classes this also covers the case where people create empty "base" classes, which is an antipattern in my opinion.

Ideally, I would also like to have a rule which flags classes with __init__ and one public classes (no decorators, no parent classes). I believe that is another antipattern, such classes are frequently better refactored to functions.

MichaReiser · 2024-09-10T20:34:54Z

@zanieb @MichaReiser Is there any progress on merging this? I think it would be a beneficial rule to add.

Not much other than that we have to come up with a good rule name that captures the motivation. Do you have any suggestions for a good rule name?

Rizhiy · 2024-09-10T21:46:42Z

Not much other than that we have to come up with a good rule name that captures the motivation. Do you have any suggestions for a good rule name?

What is the naming convention/naming rules?

IMHO, there are three different rules here, they can be named:

For empty base class: unnecessary-base-class
For dataclass case (no public methods except __init__): prefer-dataclass
For class with __init__ and one public method: potential-obfuscated-function

dylwil3 · 2025-01-03T23:06:40Z

Just some updates here:

I've made the rule rather strict, much closer to the bugbear implementation, since there seemed to be quite a few false positives in the ecosystem results. There were classes in the ecosystem results where the suggestion to replace with a namedtuple or dataclass didn't make any sense because there were things happening in the __init__ function that were too complicated to appear in either of these structures.
With that in mind, I've moved the rule to B903 and changed the name to better reflect the behavior.

Gonna check out the ecosystem results and possibly update the documentation, but otherwise I think this is close to done unless there are any objections!

Update: Ecosystem results look much better to me now!

crates/ruff_linter/src/rules/flake8_bugbear/rules/class_as_data_structure.rs

dylwil3 · 2025-01-07T03:20:58Z

Thanks for implementing this @diceroll123 and sorry for the delay on getting it over the line!

* main: Use uv consistently throughout the documentation (#15302) [red-knot] Eagerly normalize `type[]` types (#15272) [`pyupgrade`] Split `UP007` to two individual rules for `Union` and `Optional` (`UP007`, `UP045`) (#15313) [red-knot] Improve symbol-lookup tracing (#14907) [red-knot] improve type shrinking coverage in red-knot property tests (#15297) [`flake8-return`] Recognize functions returning `Never` as non-returning (`RET503`) (#15298) [`flake8-bugbear`] Implement `class-as-data-structure` (`B903`) (#9601) Avoid treating newline-separated sections as sub-sections (#15311) Remove call when removing final argument from `format` (#15309) Don't enforce `object-without-hash-method` in stubs (#15310) Don't special-case class instances in binary expression inference (#15161) Upgrade zizmor to the latest version in CI (#15300)

diceroll123 force-pushed the add-R0903 branch 2 times, most recently from 4894b8f to 366bd8e Compare January 21, 2024 22:57

diceroll123 marked this pull request as draft January 21, 2024 23:47

diceroll123 marked this pull request as ready for review January 22, 2024 00:30

diceroll123 force-pushed the add-R0903 branch 2 times, most recently from 02b14f3 to 40bc3c0 Compare January 22, 2024 00:38

MichaReiser added the rule Implementing or modifying a lint rule label Jan 24, 2024

diceroll123 force-pushed the add-R0903 branch from a4a7b17 to 29ee553 Compare January 25, 2024 01:48

diceroll123 force-pushed the add-R0903 branch 3 times, most recently from d51d701 to 514affe Compare January 25, 2024 02:06

MichaReiser reviewed Feb 26, 2024

View reviewed changes

crates/ruff_linter/src/rules/pylint/rules/too_few_public_methods.rs Outdated Show resolved Hide resolved

crates/ruff_linter/src/rules/pylint/rules/too_few_public_methods.rs Outdated Show resolved Hide resolved

AlexWaygood reviewed Feb 26, 2024

View reviewed changes

crates/ruff_linter/src/rules/pylint/rules/too_few_public_methods.rs Outdated Show resolved Hide resolved

crates/ruff_linter/src/rules/pylint/rules/too_few_public_methods.rs Outdated Show resolved Hide resolved

MichaReiser added packaging Package management related, i.e. uv accepted Ready for implementation and removed packaging Package management related, i.e. uv labels Apr 5, 2024

charliermarsh mentioned this pull request Jun 6, 2024

[New rules request] Empty class and class with two methods, one of which is init #11763

Closed

dylwil3 added 9 commits January 3, 2025 15:06

wording of docstring

80282c3

only lint if init args are non variadic and have annotations

4a5a8c2

couple more test cases

e648148

rename rule

a4e6f5e

a few more rename spots

bb0f0e7

add flake8 tests and rename fixture

24a8bf3

move rule to bugbear

3301882

inits must have only simple assignments

fa2b724

remove extra B in code and update tests

f088eda

dylwil3 force-pushed the add-R0903 branch from 591d3aa to f088eda Compare January 3, 2025 23:00

dylwil3 changed the title ~~[pylint] - Implement too-few-public-methods rule (PLR0903)~~ [flake8-bugbear] Implement class-as-data-structure rule (B903) Jan 3, 2025

dylwil3 changed the title ~~[flake8-bugbear] Implement class-as-data-structure rule (B903)~~ [flake8-bugbear] Implement class-as-data-structure (B903) Jan 3, 2025

dylwil3 requested review from AlexWaygood and MichaReiser January 6, 2025 03:10