prototype: object type extending #3379
Conversation
|
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: |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Type: Enhancement
PR Summary: This pull request introduces a new feature aimed at extending the functionality of object types within the Strawberry GraphQL framework. It adds a StrawberryObjectBuilder class that allows for customization at various stages of the object type creation process, including before wrapping a dataclass, during field processing, and after the entire process. This enhancement is designed to provide developers with more flexibility and control over how object definitions are constructed, potentially translating 'auto' types or changing field classes to custom ones. Additionally, it integrates this builder pattern into existing functions such as _process_type, type, input, and interface by allowing an optional builder parameter.
Decision: Comment
📝 Type: 'Enhancement' - not supported yet.
- Sourcery currently only approves 'Typo fix' PRs.
✅ Issue addressed: this change correctly addresses the issue or implements the desired feature.
No details provided.
✅ Small diff: the diff is small enough to approve with confidence.
No details provided.
General suggestions:
- Ensure that the new
StrawberryObjectBuilderclass and its methods are thoroughly documented, including clear examples of how and when to use them. This will help users understand the benefits and capabilities of this new feature. - Consider the impact of making the
builderparameter required in the future, as noted in the TODO comment. Evaluate whether this change would significantly improve the internal design or usability of the framework, and if so, plan for a deprecation path that minimizes disruption for users. - Review and address any linter warnings directly rather than suppressing them, to maintain code quality and readability. If suppression is absolutely necessary, provide clear comments explaining the reason.
- Clarify the purpose and expected behavior of the
after_processmethod in theStrawberryObjectBuilderclass. The current placeholder docstring ('Dont know tbh') should be replaced with a meaningful description.
Thanks for using Sourcery. We offer it for free for open source projects and would be very grateful if you could help us grow. If you like it, would you consider sharing Sourcery on your favourite social media? ✨
strawberry/object_type.py
Outdated
| Callable[[Any, GraphQLResolveInfo, GraphQLAbstractType], str] | ||
| ], | ||
| ) -> StrawberryObjectDefinition: | ||
| """Posibility to use custom StrawberryObjectDefinition""" |
There was a problem hiding this comment.
nitpick (llm): There's a typo in the docstring: 'Posibility' should be corrected to 'Possibility'.
strawberry/object_type.py
Outdated
| ) | ||
|
|
||
| def after_process(self, cls: Type[WithStrawberryObjectDefinition]): | ||
| """Dont know tbh""" |
There was a problem hiding this comment.
issue (llm): The docstring for 'after_process' is unclear and unprofessional. It should be updated to accurately describe the method's purpose or effect, even if it's currently a placeholder for future implementation.
strawberry/object_type.py
Outdated
| directives: Optional[Sequence[object]], | ||
| extend: bool, | ||
| fields: List[StrawberryField], | ||
| is_type_of: Optional[Callable[[Any, GraphQLResolveInfo], bool]], |
There was a problem hiding this comment.
suggestion (llm): Consider providing a brief explanation or example usage of 'is_type_of' and 'resolve_type' in the docstring for 'create_object_definition'. This would help users understand when and how to use these parameters effectively.
strawberry/object_type.py
Outdated
| is_interface: bool, | ||
| interfaces: List[StrawberryObjectDefinition], | ||
| description: Optional[str], | ||
| directives: Optional[Sequence[object]], |
There was a problem hiding this comment.
suggestion (llm): It might be beneficial to specify a more precise type than 'object' for the elements of 'directives'. If there's a common interface or base class that all directives should adhere to, using that would enhance type safety and code readability.
strawberry/types/type_resolver.py
Outdated
| @@ -76,6 +87,9 @@ class if one is not set by either using an explicit strawberry.field(name=...) o | |||
|
|
|||
| # then we can proceed with finding the fields for the current class | |||
| for field in dataclasses.fields(cls): # type: ignore | |||
| # Builder field hook | |||
| field = builder.on_field(field=field) # noqa: PLW2901 | |||
There was a problem hiding this comment.
suggestion (llm): The use of 'noqa: PLW2901' suggests that there's a linter warning being suppressed. It's generally a good practice to address the root cause of the warning if possible, rather than suppressing it. If this suppression is necessary, consider adding a comment explaining why.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3379 +/- ##
=======================================
Coverage 96.57% 96.57%
=======================================
Files 524 525 +1
Lines 33630 33651 +21
Branches 5578 5580 +2
=======================================
+ Hits 32479 32500 +21
+ Misses 915 914 -1
- Partials 236 237 +1 |
CodSpeed Performance ReportMerging #3379 will not alter performanceComparing Summary
|
Kitefiko
force-pushed
the
object-extension
branch
from
5cbe67a
to
4fa4129
Compare
strawberry/types/type_extension.py
Outdated
| def before_wrap_dataclass(self, cls: Type) -> None: | ||
| pass |
There was a problem hiding this comment.
Maybe we should use something similar to our extensions api and define this as on_wrap_dataclass, in which the user can yield inside, meaning he can call "before" code before it, and "after" code after it. That also allows they to except exceptions
What do you think?
There was a problem hiding this comment.
Maybe we should use something similar to our extensions api and define this as
on_wrap_dataclass, in which the user canyieldinside, meaning he can call "before" code before it, and "after" code after it. That also allows they to except exceptionsWhat do you think?
I was modeling this based on FieldExtension. Where these hooks are more similar to it's apply function. While I can imagine on_wrap_dataclass possibly being context manager for both before_wrap_dataclass & after_process, I don't see how would that work for on_field.
strawberry/types/type_extension.py
Outdated
| def on_field(self, field: Field | StrawberryField) -> Any: | ||
| return field |
|
Just rebased and adjusted so solution is no longer outdated - |
|
I like where this is going... Checking it today I'm wondering if we should alllow a list of type extensions instead of just one. Then when doing, for extension in extensions:
field = extension.on_field(field)And so on. This would allow for much more customization. What do you think about this @patrick91 ? |
yup, we do this for fields already 😊 |
There are 2 reasons why I didn't do that.
You could do something like class MyTypeExtension(TypeExtension):
def __init__(self, field_extensions: Iterable[...]):
self._field_extensions = field_extensions
def on_field(self, field: dataclasses.Field[Any]):
for ext in self._field_extensions:
field = ext.apply(field)
return fieldWhat do you guys think? Also could you explain a little more how you envision the usage of context manager styled hooks @bellini666 ? |
IMO I think it is still worth it. This only runs once, and the performance hit here should be insignificant.
Hrm, I think the code that I mentioned changed now, but I was thinking in something like this: https://github.com/strawberry-graphql/strawberry/blob/main/strawberry/extensions/base_extension.py#L27 . With that you can have some code that executes "before" and "after" without having to define 2 functions for that. But I don't think that makes sense for the |
True that, some more things to consider between single and multi I personally like single
Thanks for clarifying this. I do like context manager approach here too, after figuring out how it works. I might have added this at the latest update but, I was playing with integration into django library and thing popped up: should |
Kitefiko
force-pushed
the
object-extension
branch
from
367f8af
to
883bd49
Compare
Kitefiko
force-pushed
the
object-extension
branch
from
883bd49
to
41a44b1
Compare
There was a problem hiding this comment.
I'm a bit late to the party, but thanks for picking this up again! I see that it picks up some ideas from my initial concept, but also opens up to some changes. I've added a few comments that came to my mind immediately.
The main open point I see is wether we want to support one or multiple type extensions on the same type. My initial draft aimed for the support of multiple extensions, however I am open to other opinions and curious to hear why you chose to take this approach.
| @@ -177,7 +178,7 @@ def wrap(cls: Any) -> Type[StrawberryTypeFromPydantic[PydanticModel]]: | |||
| ) | |||
|
|
|||
| wrapped = _wrap_dataclass(cls) | |||
| extra_strawberry_fields = _get_fields(wrapped, {}) | |||
| extra_strawberry_fields = _get_fields(wrapped, TypeExtension(), {}) | |||
There was a problem hiding this comment.
Instantiating an empty type extension feels wrong to me.
There was a problem hiding this comment.
This I currently take as a temp code, I am planning on implementing PydanticTypeExtension once API is agreed upon.
| is_type_of = getattr(cls, "is_type_of", None) | ||
| resolve_type = getattr(cls, "resolve_type", None) | ||
|
|
||
| cls.__strawberry_definition__ = StrawberryObjectDefinition( | ||
| cls.__strawberry_definition__ = extension.create_object_definition( |
There was a problem hiding this comment.
I'm not sure about letting the extension influence the instantiation of the strawberry object definition. This severely limits our ability to add new parameters to the constructor later on without breaking the public API. Addtionally, this implies that only one type extension is supported on a type.
However, having the extension change all of the fields manually after creation could also lead to worse DevX. Open for other opinions on this /cc @strawberry-graphql/core
Yes. This is currently blocker right now. I would really appreciate for more people to weight in.
I have touched on this in previous comment but the main points IMO are: Single
Multiple
You could create |
Prototype for #2605