Fix info by adding a custom __class_getitem__
#3419
Conversation
|
Thanks for adding the Here's a preview of the changelog: This release properly allows passing one argument to the This is now fully supported: import strawberry
from typing import TypedDict
class Context(TypedDict):
user_id: str
@strawberry.type
class Query:
@strawberry.field
def info(self, info: strawberry.Info[Context]) -> str:
return info.context["user_id"]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.
Hey @patrick91 - I've reviewed your changes and they look great!
General suggestions:
- Consider simplifying the
__class_getitem__implementation to enhance code maintainability. - Adding a docstring to the
__class_getitem__method could improve readability and provide clarity on its purpose and usage. - Evaluate the use of
Anyas a default type for better type safety and consider documenting the rationale if it remains. - Including test cases for error conditions and edge cases, such as invoking
strawberry.Infowithout any arguments, would strengthen the test suite.
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 3 issues found
- 🟡 Complexity: 1 issue found
- 🟢 Docstrings: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
strawberry/types/info.py
Outdated
| def __class_getitem__(cls, types: Union[type, Tuple[type, ...]]) -> Type[Info]: | ||
| if not isinstance(types, tuple): | ||
| types = (types, Any) | ||
|
|
||
| return super().__class_getitem__(types) |
There was a problem hiding this comment.
suggestion (code_clarification): Consider adding a docstring for __class_getitem__.
Given the significance of this method in enabling a new way to pass arguments to the Info class, a docstring explaining its purpose, parameters, and return type would enhance code readability and maintainability.
strawberry/types/info.py
Outdated
| if not isinstance(types, tuple): | ||
| types = (types, Any) |
There was a problem hiding this comment.
question (code_refinement): Ensure Any is the intended default type.
Using Any as a default type when types is not a tuple could lead to less type-safe code. If this is intentional, consider documenting the rationale; otherwise, evaluate if a more specific default type could be used.
strawberry/types/info.py
Outdated
| @@ -38,6 +39,12 @@ | |||
| _raw_info: GraphQLResolveInfo | |||
| _field: StrawberryField | |||
|
|
|||
| def __class_getitem__(cls, types: Union[type, Tuple[type, ...]]) -> Type[Info]: | |||
| if not isinstance(types, tuple): | |||
| types = (types, Any) | |||
There was a problem hiding this comment.
suggestion (code_refinement): Consider the implications of modifying types in-place.
Modifying the types variable directly changes the method's input argument, which could lead to unexpected behavior if types is used later in the method. Although not currently the case, as a best practice, consider using a new variable for the modified tuple.
| @@ -0,0 +1,15 @@ | |||
| from typing import Any | |||
There was a problem hiding this comment.
suggestion (testing): Consider adding a test case for error conditions.
It would be beneficial to include a test case that verifies the behavior when invalid types are passed to strawberry.Info. This could help ensure that the custom __class_getitem__ method handles error conditions gracefully.
| @@ -0,0 +1,15 @@ | |||
| from typing import Any | |||
There was a problem hiding this comment.
suggestion (testing): Add a test case for using strawberry.Info without any arguments.
Including a test case for strawberry.Info invoked without any arguments could verify that the default behavior aligns with expectations. This scenario seems to be missing and could be an important edge case.
| @@ -0,0 +1,15 @@ | |||
| from typing import Any | |||
There was a problem hiding this comment.
praise (testing): Praise for covering both one and two argument use cases.
The inclusion of tests for both one and two argument scenarios for strawberry.Info is commendable. It ensures that the functionality works as expected in these common use cases.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #3419 +/- ##
==========================================
- Coverage 96.40% 95.37% -1.03%
==========================================
Files 499 500 +1
Lines 31129 31145 +16
Branches 3815 3815
==========================================
- Hits 30010 29705 -305
- Misses 913 1232 +319
- Partials 206 208 +2 |
CodSpeed Performance ReportMerging #3419 will not alter performanceComparing Summary
|
I should have tested this at runtime, woops 😅