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

Added excution_context_class for custom ExecutionContext #6

Merged
merged 2 commits into from
Sep 18, 2018

Conversation

syrusakbary
Copy link
Member

This PR is essential for having the package usable within Graphene.

Graphene needs a custom get_variable_values, get_argument_values, since the input arguments provided in the GraphQL query will be camelCase but it should be received with snake_case by the resolver.
In graphql-core we use out_name in the main implementation.
https://github.com/graphql-python/graphql-core/blob/master/graphql/execution/values.py#L110

There are some other non-performant ways to achieve this (such as decorating the resolver to snake case the args automatically), but I'm intentionally trying to avoid them since it causes a lot of performance/memory headaches.

Since we want to keep graphql-core-next closer to the javascript implementation, I think is much easier to move this logic into graphene itself, hence the need of customizing ExecutionContext.

@syrusakbary syrusakbary force-pushed the custom-execution-context branch from 238f0a1 to e49f851 Compare September 6, 2018 16:32
@coveralls
Copy link

Coverage Status

Coverage remained the same at 96.619% when pulling e49f851 on custom-execution-context into d3bdab4 on master.

@syrusakbary syrusakbary requested a review from Cito September 6, 2018 17:40
@Cito
Copy link
Member

Cito commented Sep 15, 2018

Sorry it took so long for me to review, life got in the way.

I like this way of integration that allows keeping the code close to GraphQL.js. Thoughts:

  • Instead of adding a parameter execution_context_class=ExecutionContextClass, maybe we should add a parameter execution_context_builder=ExecutionContext.build? That's actually the only thing we need.
  • You moved the execute function and I agree that it should go either at the top or at the bottom. But next time can you make two commits for changing a function and moving it? Then it's easier to compare the changes.
  • In graphql.py you made two style changes (double quotes for __all__ items and different indentation of parameters). Was this intentional, and is there a good reason to adopt this style? I personally don't mind, but I like to keep the style consistent everywhere in the same project and also compatible with PEP8 (both our styles are).

Copy link
Member

@Cito Cito left a comment

Choose a reason for hiding this comment

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

See comments above.

@syrusakbary
Copy link
Member Author

Replying inline:

Instead of adding a parameter execution_context_class=ExecutionContextClass, maybe we should add a parameter execution_context_builder=ExecutionContext.build? That's actually the only thing we need.

I was thinking something similar, but discarded since at the end the result of the build function should be ExecutionContextClass, so it simplifies it a bit. However I don't really mind to change to the builder suggestion, as I think is also a viable alternative.

You moved the execute function and I agree that it should go either at the top or at the bottom. But next time can you make two commits for changing a function and moving it? Then it's easier to compare the changes.

Will do for next time! ;)

In graphql.py you made two style changes (double quotes for __all__ items and different indentation of parameters). Was this intentional, and is there a good reason to adopt this style? I personally don't mind, but I like to keep the style consistent everywhere in the same project and also compatible with PEP8 (both our styles are).

I'm using black for uniform code formatting. It get's executed each time I save a file using VSCode.
I think might be great to integrate black into this project, as is already integrated on graphql-core, graphene, and offers an universal way of formatting code.

Thoughts?

@Cito
Copy link
Member

Cito commented Sep 18, 2018

I was thinking something similar, but discarded since at the end the result of the build function should be ExecutionContextClass, so it simplifies it a bit.

Ok, let's use your original proposal then.

I'm using black for uniform code formatting. It get's executed each time I save a file using VSCode.
I think might be great to integrate black into this project, as is already integrated on graphql-core, graphene, and offers an universal way of formatting code.

Thoughts?

I haven't used black so far. Just had a look and I'm not so excited about the 88 char limit and using double quotes by default which both go against common Python conventions - PEP 8 recommends 79 chars and repr() outputs single quotes. OTOH, not having to care about formatting and consistency throughout the GraphQL projects is more important. I think I'll make the switch and convert everything to black conventions then.

@Cito Cito merged commit a003063 into master Sep 18, 2018
@Cito Cito deleted the custom-execution-context branch September 18, 2018 11:41
Cito added a commit that referenced this pull request Sep 19, 2018
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.

3 participants