-
-
Notifications
You must be signed in to change notification settings - Fork 593
Resolve TODO: Replace EnumDefinition with StrawberryEnum #3999
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Reviewer's GuideThis PR fully replaces the deprecated EnumDefinition with the newly named StrawberryEnum across the entire codebase by renaming the core class, updating exports, refactoring imports, type hints, isinstance checks, and aligning tests accordingly. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3999 +/- ##
==========================================
+ Coverage 94.24% 94.25% +0.01%
==========================================
Files 531 532 +1
Lines 34829 34876 +47
Branches 1847 1847
==========================================
+ Hits 32824 32872 +48
Misses 1700 1700
+ Partials 305 304 -1 🚀 New features to boost your workflow:
|
CodSpeed Performance ReportMerging #3999 will create unknown performance changesComparing Summary
Footnotes
|
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `RELEASE.md:3` </location>
<code_context>
+Release type: patch
+
+Resolve TODO's about EnumDefinition by renaming it to StrawberryEnum
</code_context>
<issue_to_address>
**suggestion (typo):** Consider changing "TODO's" to "TODOs" for correct pluralization.
The correct plural is "TODOs" without the apostrophe.
</issue_to_address>
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Thanks for adding the Here's a preview of the changelog: Deprecation Alert
Other Changes
Here's the tweet text:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Summary
This PR performs a systematic codebase-wide refactoring that renames the EnumDefinition
class to StrawberryEnum
and updates all references throughout the Strawberry GraphQL library. The change affects 17 files across core modules, tests, and integrations, resolving existing TODO comments that had explicitly marked this renaming as planned technical debt.
The refactoring touches several key areas of the codebase:
- Core enum handling: The main enum class in
strawberry/types/enum.py
is renamed fromEnumDefinition
toStrawberryEnum
, with the__all__
export list updated accordingly - Schema system: Updates to schema converters, name converters, and type checking logic to use the new class name
- Code generation: Query codegen and federation modules updated to properly handle the renamed enum type
- Type system: Updates to argument handling, annotation processing, and printer logic
- Integrations: Pydantic integration updated to work with the new enum class name
- Test suite: Comprehensive updates to test assertions and mock object creation
The change maintains complete backward compatibility since StrawberryEnum
provides the identical interface and functionality as EnumDefinition
. The renaming aligns with Strawberry's naming conventions where core types are prefixed with 'Strawberry' (similar to StrawberryType
, StrawberryUnion
, etc.). All functionality remains exactly the same - this is purely a naming standardization effort that removes deprecated references and improves code consistency across the library.
Confidence score: 5/5
- This PR is extremely safe to merge with no risk of breaking functionality
- Score reflects that this is a systematic renaming with identical interfaces and comprehensive coverage across all affected files
- No files require special attention as the changes are mechanical and maintain exact same behavior
19 files reviewed, no comments
…instead of _enum_definition
for more information, see https://pre-commit.ci
… in __strawberry_definition__
for more information, see https://pre-commit.ci
…berryObjectDefinition
for more information, see https://pre-commit.ci
values.append(value) | ||
|
||
cls._enum_definition = EnumDefinition( # type: ignore | ||
cls.__strawberry_definition__ = StrawberryEnum( # type: ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we also keep a deprecated ._enum_definiton
for compatibility?
You can use this DeprecatedDescriptor, like we have for the old _type_definition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, I'll do it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated!
|
||
@dataclasses.dataclass | ||
class EnumDefinition(StrawberryType): | ||
class StrawberryEnum(StrawberryType): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like my comment below, we also need to expose the deprecated version of this
Like in https://github.com/strawberry-graphql/strawberry/blob/main/strawberry/types/base.py#L456-L456
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, updating now!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated, @bellini666 , I use the same implementation we done in this PR #2836 .
Please let me now if you see a better way to do it!
RELEASE.md
Outdated
@@ -0,0 +1,3 @@ | |||
Release type: patch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this be a minor? It does more than just fixing, it also changes variable names
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree 100% with you, also will add a deprecated info on the release
Already fixing this CI errors. |
Ready to review @bellini666 |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Description
Replace
EnumDefinition
withStrawberryEnum
Types of Changes
Issues Fixed or Closed by This PR
Checklist
Summary by Sourcery
Replace the legacy EnumDefinition with StrawberryEnum throughout the core codebase, ensuring all conversions, codegen paths, and tests are updated to use the new class name.
Enhancements:
Documentation: