Add no-redundant-dataclasses pre-commit hook
#4058
Open
+146
−0
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Reviewer's GuideAdds a new AST-based linter and pre-commit hook that fails when Strawberry GraphQL types are redundantly decorated with dataclasses.dataclass, wires it up as a CLI entry point, and tests the behavior. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
magicmark
force-pushed
the
no-redundant-dataclasses_pre-commit-hook
branch
from
ac8596f to
0430540
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4058 +/- ##
==========================================
+ Coverage 94.41% 94.42% +0.01%
==========================================
Files 536 539 +3
Lines 35036 35132 +96
Branches 1842 1848 +6
==========================================
+ Hits 33079 33175 +96
Misses 1659 1659
Partials 298 298 🚀 New features to boost your workflow:
|
magicmark
force-pushed
the
no-redundant-dataclasses_pre-commit-hook
branch
from
0430540 to
e6c1e18
Compare
magicmark
changed the title
no-redundant-dataclasses pre-commit hook
magicmark
force-pushed
the
no-redundant-dataclasses_pre-commit-hook
branch
from
e6c1e18 to
1ea803c
Compare
magicmark
force-pushed
the
no-redundant-dataclasses_pre-commit-hook
branch
from
1ea803c to
8fa7e63
Compare
Member
|
Thanks for adding the Here's a preview of the changelog: Add a pre-commit hook to prevent redundant Since Strawberry types ( To use the hook, add it to your repos:
- repo: https://github.com/strawberry-graphql/strawberry
rev: <version>
hooks:
- id: no-redundant-dataclassesHere's the tweet text: |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In
no_redundant_dataclasses.mainyou open files in binary mode and pass the raw bytes toast.parse, which expects a string; switch to text mode with an explicit encoding (e.g.open(filename, encoding="utf-8")) or decode the bytes before parsing to avoid runtime errors on some Python versions. - Consider gracefully handling
SyntaxErrorwhen parsing files withast.parseso that the hook reports a clear message (or skips invalid files) instead of crashing the pre-commit run on syntactically invalid Python. - The visitor currently only matches
@strawberry.type; if other Strawberry decorators (e.g.@strawberry.input,@strawberry.interface) should also forbiddataclasses.dataclass, it might be worth generalizing the decorator check to a configurable or broader set of Strawberry decorators.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `no_redundant_dataclasses.main` you open files in binary mode and pass the raw bytes to `ast.parse`, which expects a string; switch to text mode with an explicit encoding (e.g. `open(filename, encoding="utf-8")`) or decode the bytes before parsing to avoid runtime errors on some Python versions.
- Consider gracefully handling `SyntaxError` when parsing files with `ast.parse` so that the hook reports a clear message (or skips invalid files) instead of crashing the pre-commit run on syntactically invalid Python.
- The visitor currently only matches `@strawberry.type`; if other Strawberry decorators (e.g. `@strawberry.input`, `@strawberry.interface`) should also forbid `dataclasses.dataclass`, it might be worth generalizing the decorator check to a configurable or broader set of Strawberry decorators.
## Individual Comments
### Comment 1
<location> `strawberry/hooks/no_redundant_dataclasses.py:22-31` </location>
<code_context>
+ is_strawberry_class = False
+ is_raw_dataclass = False
+
+ for decorator in node.decorator_list:
+ if (
+ isinstance(decorator, ast.Attribute)
+ and isinstance(decorator.value, ast.Name)
+ and decorator.value.id == "strawberry"
+ and decorator.attr == "type"
+ ) or (
+ isinstance(decorator, ast.Call)
+ and isinstance(decorator.func, ast.Attribute)
+ and isinstance(decorator.func.value, ast.Name)
+ and decorator.func.value.id == "strawberry"
+ and decorator.func.attr == "type"
+ ):
+ is_strawberry_class = True
+
+ if (isinstance(decorator, ast.Name) and decorator.id == "dataclass") or (
+ isinstance(decorator, ast.Attribute)
+ and isinstance(decorator.value, ast.Name)
</code_context>
<issue_to_address>
**suggestion:** Current detection only covers `strawberry.type` and misses other Strawberry decorators that also imply dataclasses.
The detection logic only flags `@strawberry.type` / `@strawberry.type()` as Strawberry classes. Other decorators like `@strawberry.input`, `@strawberry.interface`, etc. are also dataclass-based and should likely be treated the same way. Consider generalizing `is_strawberry_class` to match any relevant `strawberry.<...>` decorator (possibly via a small allowlist) rather than hard-coding `attr == "type"`, so the hook also covers these cases.
</issue_to_address>
### Comment 2
<location> `strawberry/hooks/no_redundant_dataclasses.py:58-59` </location>
<code_context>
+ violations: list[str] = []
+
+ for filename in args.filenames:
+ with Path(filename).open("rb") as f:
+ tree = ast.parse(f.read(), filename=filename)
+ visitor = Visitor()
+ visitor.visit(tree)
</code_context>
<issue_to_address>
**issue (bug_risk):** Consider handling `SyntaxError` (and possibly `UnicodeDecodeError`) when parsing files so the hook degrades gracefully.
Right now, a syntax or decoding issue in any file will cause `ast.parse` to raise and abort the whole hook run. It would be better to catch `SyntaxError` (and possibly `UnicodeError`) around the parse, then either record a violation for that file or skip it with a clear diagnostic, so other files can still be processed.
</issue_to_address>
### Comment 3
<location> `tests/hooks/test_no_redundant_dataclasses.py:6-15` </location>
<code_context>
+from strawberry.hooks.no_redundant_dataclasses import main
+
+
+def test_check_namespaced_decorator(tmp_path):
+ code = """
+ import strawberry
+ import dataclasses
+
+ @dataclasses.dataclass
+ @strawberry.type
+ class Foo: ...
+ """
+ file = tmp_path / "foo.py"
+ file.write_text(dedent(code), encoding="utf-8")
+ exit_code = main([str(file)])
+ assert exit_code == 1
+
+
</code_context>
<issue_to_address>
**issue (testing):** Add tests for `@dataclasses.dataclass()` / `@dataclass()` call-style decorators
The current test only covers `dataclass` used as a bare decorator, but not when it’s called (e.g. `@dataclasses.dataclass()` or `@dataclass(eq=False)`). Since call-style usage is common, please add tests like:
```python
@dataclasses.dataclass()
@strawberry.type
class Foo: ...
```
and
```python
from dataclasses import dataclass
@dataclass(eq=False)
@strawberry.type
class Foo: ...
```
to ensure the behavior is clearly defined and to catch any discrepancies for call-style dataclass decorators.
</issue_to_address>
### Comment 4
<location> `tests/hooks/test_no_redundant_dataclasses.py:36-45` </location>
<code_context>
+ assert exit_code == 1
+
+
+def test_check_passing_file(tmp_path):
+ code = """
+ import strawberry
+
+ @strawberry.type
+ class Foo: ...
+ """
+ file = tmp_path / "foo.py"
+ file.write_text(dedent(code), encoding="utf-8")
+ exit_code = main([str(file)])
+ assert exit_code == 0
</code_context>
<issue_to_address>
**suggestion (testing):** Add negative tests to ensure non-strawberry dataclasses are not flagged
Right now we only cover the positive Strawberry case. Please also add tests with a plain dataclass (both `import dataclasses` and `from dataclasses import dataclass`) with no Strawberry decorator, and assert `exit_code == 0` in both. This will confirm the hook only flags *combined* dataclass + Strawberry types, not any dataclass usage.
</issue_to_address>
### Comment 5
<location> `strawberry/hooks/no_redundant_dataclasses.py:64` </location>
<code_context>
def main(argv: Sequence[str] | None = None) -> int:
parser = argparse.ArgumentParser()
parser.add_argument("filenames", nargs="*")
args = parser.parse_args(argv)
violations: list[str] = []
for filename in args.filenames:
with Path(filename).open("rb") as f:
tree = ast.parse(f.read(), filename=filename)
visitor = Visitor()
visitor.visit(tree)
violations.extend(f"- {filename}{error}" for error in visitor.errors)
if len(violations) == 0:
return 0
msg = "\n".join(
(
"Decorating strawberry types with dataclasses.dataclass is redundant.",
"Remove the dataclass decorator from the following classes:",
*violations,
)
)
print(msg)
return 1
</code_context>
<issue_to_address>
**suggestion (code-quality):** Simplify sequence length comparison ([`simplify-len-comparison`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/simplify-len-comparison/))
```suggestion
if not violations:
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+58
to
+59
| with Path(filename).open("rb") as f: | ||
| tree = ast.parse(f.read(), filename=filename) |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Consider handling SyntaxError (and possibly UnicodeDecodeError) when parsing files so the hook degrades gracefully.
Right now, a syntax or decoding issue in any file will cause ast.parse to raise and abort the whole hook run. It would be better to catch SyntaxError (and possibly UnicodeError) around the parse, then either record a violation for that file or skip it with a clear diagnostic, so other files can still be processed.
Comment on lines
+6
to
+15
| def test_check_namespaced_decorator(tmp_path): | ||
| code = """ | ||
| import strawberry | ||
| import dataclasses | ||
|
|
||
| @dataclasses.dataclass | ||
| @strawberry.type | ||
| class Foo: ... | ||
| """ | ||
| file = tmp_path / "foo.py" |
Contributor
There was a problem hiding this comment.
issue (testing): Add tests for @dataclasses.dataclass() / @dataclass() call-style decorators
The current test only covers dataclass used as a bare decorator, but not when it’s called (e.g. @dataclasses.dataclass() or @dataclass(eq=False)). Since call-style usage is common, please add tests like:
@dataclasses.dataclass()
@strawberry.type
class Foo: ...and
from dataclasses import dataclass
@dataclass(eq=False)
@strawberry.type
class Foo: ...to ensure the behavior is clearly defined and to catch any discrepancies for call-style dataclass decorators.
Collaborator
There was a problem hiding this comment.
I agree with this comment @magicmark and I see that you added a emoji too.
I created some tests to check it its is happen and looks like we dont show anything in this cases:
def test_check_dataclass_with_empty_parentheses(tmp_path):
code = """
import strawberry
from dataclasses import dataclass
@dataclass()
@strawberry.type
class Foo: ...
"""
file = tmp_path / "foo.py"
file.write_text(dedent(code), encoding="utf-8")
exit_code = main([str(file)])
assert exit_code == 1
def test_check_dataclass_with_parameters(tmp_path):
code = """
import strawberry
from dataclasses import dataclass
@dataclass(frozen=True)
@strawberry.type
class Foo: ...
"""
file = tmp_path / "foo.py"
file.write_text(dedent(code), encoding="utf-8")
exit_code = main([str(file)])
assert exit_code == 1
def test_check_namespaced_dataclass_with_parentheses(tmp_path):
code = """
import strawberry
import dataclasses
@dataclasses.dataclass()
@strawberry.type
class Foo: ...
"""
file = tmp_path / "foo.py"
file.write_text(dedent(code), encoding="utf-8")
exit_code = main([str(file)])
assert exit_code == 1
def test_check_namespaced_dataclass_with_parameters(tmp_path):
code = """
import strawberry
import dataclasses
@dataclasses.dataclass(frozen=True)
@strawberry.type
class Foo: ...
"""
file = tmp_path / "foo.py"
file.write_text(dedent(code), encoding="utf-8")
exit_code = main([str(file)])
assert exit_code == 1What you think adding this tests and updating the code to handle that?
I will add another comment in test_redundant_dataclasses.py with the changes I've made to fix it 😉.
Contributor
There was a problem hiding this comment.
These tests look great to me and they cover exactly the call-style cases I was worried about: empty parentheses and parameterized decorators, both for the direct @dataclass import and the namespaced @dataclasses.dataclass form.
I’d definitely be in favor of adding them, along with the corresponding code changes you mentioned in test_redundant_dataclasses.py to make the hook actually flag these patterns. Once the implementation is updated so that all four new tests fail before the change and pass after it, we’ll have really solid coverage of the decorator variants people are likely to use.
When you push the changes, I’ll re-review with the updated implementation and tests, but conceptually: yes, please go ahead and add these tests and handle those cases in the hook logic.
Contributor
Greptile OverviewGreptile SummaryAdds a pre-commit hook to detect and prevent redundant
Confidence Score: 4/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant PC as Pre-commit
participant Hook as no_redundant_dataclasses
participant AST as AST Parser
participant Visitor as Visitor
PC->>Hook: Execute with file paths
loop For each file
Hook->>AST: Parse Python file
AST-->>Hook: Return AST tree
Hook->>Visitor: Visit tree nodes
Visitor->>Visitor: Check ClassDef nodes
alt Has @strawberry.type AND @dataclass
Visitor->>Visitor: Record error
end
Visitor-->>Hook: Return errors list
end
alt Violations found
Hook->>PC: Exit code 1 + error message
else No violations
Hook->>PC: Exit code 0
end
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
CodSpeed Performance ReportMerging #4058 will not alter performanceComparing Summary
|
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Contributor
Author
|
@patrick91 ready for review! all checks passing now. |
Ckk3
suggested changes
Dec 6, 2025
Comment on lines
+6
to
+15
| def test_check_namespaced_decorator(tmp_path): | ||
| code = """ | ||
| import strawberry | ||
| import dataclasses | ||
|
|
||
| @dataclasses.dataclass | ||
| @strawberry.type | ||
| class Foo: ... | ||
| """ | ||
| file = tmp_path / "foo.py" |
Collaborator
There was a problem hiding this comment.
I agree with this comment @magicmark and I see that you added a emoji too.
I created some tests to check it its is happen and looks like we dont show anything in this cases:
def test_check_dataclass_with_empty_parentheses(tmp_path):
code = """
import strawberry
from dataclasses import dataclass
@dataclass()
@strawberry.type
class Foo: ...
"""
file = tmp_path / "foo.py"
file.write_text(dedent(code), encoding="utf-8")
exit_code = main([str(file)])
assert exit_code == 1
def test_check_dataclass_with_parameters(tmp_path):
code = """
import strawberry
from dataclasses import dataclass
@dataclass(frozen=True)
@strawberry.type
class Foo: ...
"""
file = tmp_path / "foo.py"
file.write_text(dedent(code), encoding="utf-8")
exit_code = main([str(file)])
assert exit_code == 1
def test_check_namespaced_dataclass_with_parentheses(tmp_path):
code = """
import strawberry
import dataclasses
@dataclasses.dataclass()
@strawberry.type
class Foo: ...
"""
file = tmp_path / "foo.py"
file.write_text(dedent(code), encoding="utf-8")
exit_code = main([str(file)])
assert exit_code == 1
def test_check_namespaced_dataclass_with_parameters(tmp_path):
code = """
import strawberry
import dataclasses
@dataclasses.dataclass(frozen=True)
@strawberry.type
class Foo: ...
"""
file = tmp_path / "foo.py"
file.write_text(dedent(code), encoding="utf-8")
exit_code = main([str(file)])
assert exit_code == 1What you think adding this tests and updating the code to handle that?
I will add another comment in test_redundant_dataclasses.py with the changes I've made to fix it 😉.
| ): | ||
| is_strawberry_class = True | ||
|
|
||
| if (isinstance(decorator, ast.Name) and decorator.id == "dataclass") or ( |
Collaborator
There was a problem hiding this comment.
suggestion (non-blocking)
@magicmark What you think about also detect when dataclass() is called with parentheses (empty or with args like dataclass(frozen=True))?
Please know that this was also mentioned in this sourcery comment where I added some tests to see if this is happening
Here is the code I've made that adds validations to this cases:
# Check for dataclass decorator (with or without parentheses)
decorator_node = (
decorator.func if isinstance(decorator, ast.Call) else decorator
)
if (
isinstance(decorator_node, ast.Name) and decorator_node.id == "dataclass"
) or (
isinstance(decorator_node, ast.Attribute)
and isinstance(decorator_node.value, ast.Name)
and decorator_node.value.id == "dataclasses"
and decorator_node.attr == "dataclass"
):
is_raw_dataclass = True
Member
|
shall we document this? 😊 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As promised! Per #3863.
This PR adds a
.pre-commit-hooks.yamlsetup and our first hook,no-redundant-dataclasses.Validation
(tested locally)
Checklist
Summary by Sourcery
Add a Python-based linter hook to pre-commit that flags Strawberry GraphQL types redundantly decorated with dataclasses.dataclass.
New Features:
Tests: