[flake8-bugbear] Implement class-as-data-structure (B903)

[diceroll123]

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]

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]

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]

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?

[AlexWaygood]

@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]

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]

Maybe something like NonDataclassSimpleClass?

[MichaReiser]

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

[AlexWaygood]

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]

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

[MichaReiser]

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]

@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]

@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]

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]

Just some updates here:

1. 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.
2. 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!

[dylwil3]

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