-
-
Notifications
You must be signed in to change notification settings - Fork 536
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
add optional extensions to GraphQLHTTPResponse #903
Conversation
Not sure of the best place to add tests for this? |
Thanks for adding the Here's a preview of the changelog: This release adds an extensions field to the This field gets populated by Strawberry extensions: https://strawberry.rocks/docs/guides/extensions#get-results |
Codecov Report
@@ Coverage Diff @@
## main #903 +/- ##
==========================================
+ Coverage 97.07% 97.51% +0.43%
==========================================
Files 72 76 +4
Lines 2430 2737 +307
Branches 345 382 +37
==========================================
+ Hits 2359 2669 +310
+ Misses 43 42 -1
+ Partials 28 26 -2 |
@lijok looks good to me! I think we should add test for all our integrations, what do you think? :) |
This is what I was wondering, where best would it be to add tests for these? |
new ones 😊 |
@patrick91 this should be ready now |
@@ -10,12 +10,15 @@ | |||
class GraphQLHTTPResponse(TypedDict, total=False): | |||
data: Optional[Dict[str, Any]] | |||
errors: Optional[List[Any]] | |||
extensions: Optional[Dict[str, Any]] |
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.
This is marked Optional
but can never be None
. Should we be setting data["extensions"] = None
if there aren't any? (this would apply to data
and errors
too.)
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.
data
and errors
already work like that, for extensions we always return a dict:
strawberry/strawberry/extensions/runner.py
Lines 50 to 56 in 98b4e67
def get_extensions_results(self) -> Dict[str, Any]: | |
data = {} | |
for extension in self.extensions: | |
data.update(extension.get_results()) | |
return data |
I think we can keep them as optional, I'm not sure if we will change the dict to None at somepoint (for example if there's no extension). So I'd say this is fine :)
We can always update this in another PR :)
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.
Looks good to me 😊 I think we can merge it!
Feel free to request a sticker here: https://forms.gle/dmnfQUPoY5gZbVT67 ✨
@@ -10,12 +10,15 @@ | |||
class GraphQLHTTPResponse(TypedDict, total=False): | |||
data: Optional[Dict[str, Any]] | |||
errors: Optional[List[Any]] | |||
extensions: Optional[Dict[str, Any]] |
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.
data
and errors
already work like that, for extensions we always return a dict:
strawberry/strawberry/extensions/runner.py
Lines 50 to 56 in 98b4e67
def get_extensions_results(self) -> Dict[str, Any]: | |
data = {} | |
for extension in self.extensions: | |
data.update(extension.get_results()) | |
return data |
I think we can keep them as optional, I'm not sure if we will change the dict to None at somepoint (for example if there's no extension). So I'd say this is fine :)
We can always update this in another PR :)
@patrick91 I think #589 can be closed now. |
Description
Types of Changes
Issues Fixed or Closed by This PR
Checklist