-
-
Notifications
You must be signed in to change notification settings - Fork 577
fix: codegen generic types on selection #3925
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?
fix: codegen generic types on selection #3925
Conversation
Reviewer's GuideThis PR introduces a new Class diagram for refactored generic type name resolution in codegenclassDiagram
class QueryCodegen {
_field_from_selection(selection, parent_type)
_field_from_selection_set(selection, parent_type)
_parent_type_name(parent_type) str
}
QueryCodegen : - _field_from_selection now uses _parent_type_name
QueryCodegen : - _field_from_selection_set now uses _parent_type_name
QueryCodegen : + _parent_type_name centralizes generic type name logic
class StrawberryObjectDefinition {
type_var_map
name
}
QueryCodegen --> StrawberryObjectDefinition : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
PR Summary
Fixed codegen bug where generic type renaming was not being handled consistently in query processing. The fix extracts and standardizes type name handling logic across the codebase.
- Extracted parent type name handling into new
_parent_type_name
method instrawberry/codegen/query_codegen.py
to ensure consistent generic type handling - Fixed bug where generic type renaming was skipped for selections without selection sets
- Improved code maintainability by reusing type name logic between
_field_from_selection
and_field_from_selection_set
1 file reviewed, 1 comment
Edit PR Review Bot Settings | Greptile
strawberry/codegen/query_codegen.py
Outdated
def _parent_type_name(self, parent_type: StrawberryObjectDefinition) -> str: | ||
# Check if the parent type is generic. | ||
# This seems to be tracked by `strawberry` in the `type_var_map` | ||
# If the type is generic, then the strawberry generated schema | ||
# naming convention is <GenericType,...><ClassName> | ||
# The implementation here assumes that the `type_var_map` is ordered, | ||
# but insertion order is maintained in python3.6+ (for CPython) and | ||
# guaranteed for all python implementations in python3.7+, so that | ||
# should be pretty safe. | ||
if parent_type.type_var_map: | ||
return "".join( | ||
c.__name__ # type: ignore[union-attr] | ||
for c in parent_type.type_var_map.values() | ||
) + parent_type.name | ||
return parent_type.name |
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.
style: consider documenting the generic type handling behavior with a docstring, especially the ordering assumptions
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 @franco-gondi - I've reviewed your changes and they look great!
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Hi, thanks for contributing to Strawberry 🍓! We noticed that this PR is missing a So as soon as this PR is merged, a release will be made 🚀. Here's an example of Release type: patch
Description of the changes, ideally with some examples, if adding a new feature. Release type can be one of patch, minor or major. We use semver, so make sure to pick the appropriate type. If in doubt feel free to ask :) 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.
Hi @franco-gondi!
this looks promising, can you add test that fails on main?
This pr fixes an issue where using the codegen with some queries where generic types are involved failed.
Description
Types of Changes
Generic type renaming was not being performed when there is a selection, it was being done only on selection set
Issues Fixed or Closed by This PR
Checklist
Summary by Sourcery
Unify generic parent type naming and apply it during field selection codegen to fix generic type renaming issues
Bug Fixes:
Enhancements: