Skip to content
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: use execution_context_class attribute for GraphQLView #1398

Merged

Conversation

Arfey
Copy link
Contributor

@Arfey Arfey commented Apr 19, 2023

Simple change for using execution_context_class as class attribute

class MyGraphQLView(GraphQLView):
    execution_context_class = CustomExecutionContext

@@ -112,7 +112,7 @@ def __init__(
self.pretty = self.pretty or pretty
self.graphiql = self.graphiql or graphiql
self.batch = self.batch or batch
self.execution_context_class = execution_context_class
self.execution_context_class = execution_context_class or self.execution_context_class
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probs want to follow the same pattern as the other attributes (graphiql, batch etc):

Suggested change
self.execution_context_class = execution_context_class or self.execution_context_class
self.execution_context_class = self.execution_context_class or execution_context_class

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so this is not a good pattern. class attribute it's something like default value. when we create an instance we need to have approach to redefine it. thats why i use current order.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Arfey Nice, thats a good point - could you update the other attributes to follow that pattern instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@firaskafri
Copy link
Collaborator

@Arfey great PR!
Seems we have some testing errors?

Copy link
Collaborator

@jaw9c jaw9c left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you just need to run the pre-commit hook so that the formatter runs, nice one on the PR

@firaskafri firaskafri self-requested a review May 14, 2023 07:58
@firaskafri firaskafri merged commit 388ca41 into graphql-python:main May 24, 2023
12 checks passed
superlevure pushed a commit to loft-orbital/graphene-django that referenced this pull request Jul 19, 2023
…ython#1398)

* fix: use execution_context_class attribute for GraphQLView
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants