Conversation
Reviewer's GuideAdds conditional support for graphql-core 3.3.x while preserving compatibility with 3.2.x, including version feature flags, updated field selection logic, test skips, and CI matrix changes. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Thanks for adding the Below is the changelog that will be used for the release. Add support for graphql-core 3.3.x (currently in alpha) alongside existing 3.2.x support. This release was contributed by @bellini666 in #850 |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new
_get_selectionslogic branches onIS_GQL_32but relies on multiple# pyright: ignorecomments; consider tightening the typing (e.g. explicit imports/aliases, narrowed types, or helper wrapper functions) so these ignores are unnecessary and future changes tographql-coreAPIs are easier to catch statically. - Instead of defining
IS_GQL_33andIS_GQL_32 = not IS_GQL_33, you may want to use explicit version-range checks (e.g.>=3.2,<3.3and>=3.3) so that behavior remains well-defined ifgraphql-core4.x or a later 3.x minor release is installed.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `_get_selections` logic branches on `IS_GQL_32` but relies on multiple `# pyright: ignore` comments; consider tightening the typing (e.g. explicit imports/aliases, narrowed types, or helper wrapper functions) so these ignores are unnecessary and future changes to `graphql-core` APIs are easier to catch statically.
- Instead of defining `IS_GQL_33` and `IS_GQL_32 = not IS_GQL_33`, you may want to use explicit version-range checks (e.g. `>=3.2,<3.3` and `>=3.3`) so that behavior remains well-defined if `graphql-core` 4.x or a later 3.x minor release is installed.
## Individual Comments
### Comment 1
<location> `tests/conftest.py:13` </location>
<code_context>
import strawberry_django
from strawberry_django.optimizer import DjangoOptimizerExtension
+from strawberry_django.utils import IS_GQL_32, IS_GQL_33
from tests.utils import GraphQLTestClient
</code_context>
<issue_to_address>
**issue (testing):** Missing tests that validate the behavior of the suite under both graphql-core 3.2.x and 3.3.x, especially around the optimizer selection logic.
You’ve introduced `IS_GQL_32`/`IS_GQL_33` into the test setup, but there are no tests that actually assert the optimizer’s behavior under each version. Since `_get_selections` now branches on these flags, please add a parametrized test that runs once under 3.2.x and once under 3.3.x, confirming that: (1) 3.2.x preserves the current behavior, and (2) 3.3.x with `collect_subfields` + `FieldDetails` yields the same effective selection set (including fragments/variables).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
a8a3abf to
ffbddd4
Compare
46850d9 to
8f7f461
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #850 +/- ##
==========================================
- Coverage 91.38% 91.30% -0.08%
==========================================
Files 45 47 +2
Lines 4421 4441 +20
==========================================
+ Hits 4040 4055 +15
- Misses 381 386 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
8f7f461 to
990dc3d
Compare
990dc3d to
ad57713
Compare
Summary by Sourcery
Add compatibility with graphql-core 3.3.x while retaining support for 3.2.x and update tooling accordingly.
New Features:
Enhancements: