-
Notifications
You must be signed in to change notification settings - Fork 181
Feature/fix deprecation warnings #210
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
Feature/fix deprecation warnings #210
Conversation
FYI @syrusakbary |
@@ -72,7 +72,7 @@ def batch_load_fn(self, keys): | |||
class Context(object): | |||
business_data_loader = BusinessDataLoader() | |||
|
|||
result = execute(schema, doc_ast, None, context_value=Context(), executor=executor) | |||
result = execute(schema, doc_ast, None, context=Context(), executor=executor) |
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.
What's the purpose of removing _value
from a bunch of the kwargs throughout this review? Is there some issue related to this where that's explained? A personal preference?
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.
Unless one has a foo and a foo_value, the suffix seems confusing. This is a Context instance, so the lower-case variable name seems a good fit
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.
The reason for this change is that these parameters have been renamed in graphql-core where the _value
postfix was removed in June 2018 and you get deprecation warnings now if you use the _value
postfix. That's why @ekampf removed the postfixes here as well.
However, I think we should revert the removal of _value
in graphql-core - see #234.
Closing this in favor of #234. |
No description provided.