-
-
Notifications
You must be signed in to change notification settings - Fork 547
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
fix(fastapi): context_getter signature to allow Awaitable #3763
fix(fastapi): context_getter signature to allow Awaitable #3763
Conversation
Reviewer's Guide by SourceryThe pull request modifies the signature of the Sequence diagram showing context getter usage in FastAPI routersequenceDiagram
participant Client
participant Router as GraphQLRouter
participant ContextGetter
Client->>Router: GraphQL Request
Router->>ContextGetter: Get Context
alt Synchronous Context
ContextGetter-->>Router: Return Context directly
else Asynchronous Context
ContextGetter-->>Router: Return Awaitable[Context]
Router-->>Router: Await Context
end
Router-->>Client: GraphQL Response
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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 @alexey-pelykh - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding tests to verify both synchronous and asynchronous context getters work as expected.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Thanks for adding the Here's a preview of the changelog: This release adjusts the Here's the tweet text:
|
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:
|
39e0aad
to
d2eaad7
Compare
3f17516
to
6889e9a
Compare
166a077
to
9ae8b3d
Compare
@bellini666 I force-pushed and lost your changes to RELEASE.md - was that only about that suggestion? |
yep! I didn't know you were going to be that fast, so I applied it myself lol |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3763 +/- ##
==========================================
- Coverage 97.27% 97.24% -0.03%
==========================================
Files 504 502 -2
Lines 33480 33460 -20
Branches 5502 1702 -3800
==========================================
- Hits 32566 32537 -29
- Misses 703 706 +3
- Partials 211 217 +6 |
CodSpeed Performance ReportMerging #3763 will not alter performanceComparing Summary
|
|
Description
The
context_getter
is further passed tostrawberry/strawberry/fastapi/router.py
Lines 69 to 71 in 7ba5928
strawberry/strawberry/fastapi/router.py
Line 128 in 7ba5928
Types of Changes
Issues Fixed or Closed by This PR
Checklist
Summary by Sourcery
Bug Fixes: