Unpack list type when looking up namespaces #3388
Conversation
|
Thanks for adding the Here's a preview of the changelog: Fixes an issue where lazy annotations raised an error when used together with a List Here's the tweet text: |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #3388 +/- ##
==========================================
- Coverage 96.41% 95.33% -1.08%
==========================================
Files 498 500 +2
Lines 31133 31174 +41
Branches 3814 3813 -1
==========================================
- Hits 30016 29719 -297
- Misses 911 1246 +335
- Partials 206 209 +3 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Type: Bug fix
PR Summary: This pull request addresses a specific issue where using lazy annotations with lists in type definitions caused an UnresolvedFieldTypeError. It introduces changes to the namespace resolution logic to properly handle list types, ensuring that types like List[Annotated[MyType, strawberry.lazy("my.module")]] are correctly resolved without errors.
Decision: Comment
📝 Type: 'Bug fix' - 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:
- Consider verifying the type of
expr_slicebefore passing it to_get_namespace_from_astto ensure robustness against unexpected input types. - The use of
castwithout utilizing its result seems unnecessary. If it's meant for type checking, consider applying it in a way that influences the program or removing it if it's redundant. - Overall, the changes are well-targeted and address the issue effectively. It's recommended to ensure that all edge cases and potential type mismatches are handled to prevent any unforeseen errors.
strawberry/utils/typing.py: Consolidate condition checks for "list", "List", and "Annotated" to simplify and reduce code complexity.
While reviewing the proposed changes, I noticed an opportunity to simplify the added complexity by consolidating similar conditions. The new code introduces separate branches for checking instances of ast.Subscript and ast.Name, specifically for "list" and "List", which closely mirrors the existing structure for handling "Annotated".
To enhance maintainability and readability, consider merging these conditions into a single block. This approach not only reduces repetition but also keeps the codebase concise, making it easier for future developers to navigate and understand. Here's a suggested revision that consolidates the handling of these cases:
elif isinstance(expr, ast.Subscript) and isinstance(expr.value, ast.Name):
if expr.value.id in {"list", "List", "Annotated"}:
expr_slice = expr.slice
extra.update(_get_namespace_from_ast(expr_slice, globalns, localns))This modification aims to streamline the logic by grouping similar conditions, potentially reducing the cognitive load required to grasp the code's intent and making the overall structure more elegant.
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/utils/typing.py
Outdated
| and expr.value.id in {"list", "List"} | ||
| ): | ||
| expr_slice = expr.slice | ||
| cast(ast.Tuple, expr_slice) |
There was a problem hiding this comment.
issue (llm): The use of cast here does not alter the program's behavior but is intended for type checking. However, without assigning its result or using it in a type hint, this line has no effect. Consider either removing this line if it's unnecessary or using the cast result if it was intended for type checking.
strawberry/utils/typing.py
Outdated
| ): | ||
| expr_slice = expr.slice | ||
| cast(ast.Tuple, expr_slice) | ||
| extra.update(_get_namespace_from_ast(expr_slice, globalns, localns)) |
There was a problem hiding this comment.
suggestion (llm): This update operation assumes that expr_slice will always be a valid input for _get_namespace_from_ast. Given that expr_slice comes from parsing AST nodes, it might be prudent to ensure that expr_slice is of a type that _get_namespace_from_ast can handle, or to handle potential exceptions that could arise from unexpected types.
CodSpeed Performance ReportMerging #3388 will not alter performanceComparing Summary
|
…o fix-list-annotation
jeich
force-pushed
the
fix-list-annotation
branch
from
d01483f
to
b310b16
Compare
There are some good tests on this PR for this scenario, should I add the commits to this PR? |
|
@jeich yes please 😊 |
|
Added tests from @ddoroshev |
Description
The namespace resolver does not check for lists, meaning a type definition of
List[Annotated[MyType, strawberry.lazy("my.module")]]throwsstrawberry.exceptions.unresolved_field_type.UnresolvedFieldTypeErrorTypes of Changes
Issues Fixed or Closed by This PR
List[]#2896Checklist
test_lazy_forward_reference_schema_with_a_list_onlyintroduced in Failing test for lazy lists #2895