-
-
Notifications
You must be signed in to change notification settings - Fork 959
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 graphql deprecation warnings #1056
Fix graphql deprecation warnings #1056
Conversation
@@ -123,19 +123,19 @@ async def execute( # type: ignore | |||
if self.is_async: | |||
return await self.schema.execute( | |||
query, | |||
variables=variables, | |||
variable_values=variables, |
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.
you might instead choose to propagate the rename all the way up?
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.
🤔 I'm in two minds about this. On the one hand, graphene/graphql-core seems to have settled on these names with _value
. OTOH, whether we want to properly support GraphQL in Starlette is unclear (#619) so I'm hesitant to make more than the most minimal changes to make the warnings go away.
How about we do go to the effort of renaming the parameters of this function (execute(..., variable_values=None, ...
) so that it remains a very small wrapper around calling the schema? But not do anything more than that (although that might be everything there is to do anyway 😅). It doesn't seem to be a documented Starlette API anyway, so it should be safe to change.
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.
Thanks for following up on this.
@@ -123,19 +123,19 @@ async def execute( # type: ignore | |||
if self.is_async: | |||
return await self.schema.execute( | |||
query, | |||
variables=variables, | |||
variable_values=variables, |
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.
🤔 I'm in two minds about this. On the one hand, graphene/graphql-core seems to have settled on these names with _value
. OTOH, whether we want to properly support GraphQL in Starlette is unclear (#619) so I'm hesitant to make more than the most minimal changes to make the warnings go away.
How about we do go to the effort of renaming the parameters of this function (execute(..., variable_values=None, ...
) so that it remains a very small wrapper around calling the schema? But not do anything more than that (although that might be everything there is to do anyway 😅). It doesn't seem to be a documented Starlette API anyway, so it should be safe to change.
Thank you for creating this pull request. We have decided to deprecate GraphQL support within Starlette itself so I am going to close this pull request. See #619. |
No description provided.