-
Notifications
You must be signed in to change notification settings - Fork 822
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
Discussion - Pass context to resolve methods #164
Comments
That PR seems to address this issue for me, and I've got it working locally. If there should be anything else on there, let me know. |
@mjtamlyn @danielfarrell thanks for opening the issue and the PR. For achieve more simplicity, The current implementation only uses PS: In JS this is a little bit easier as you could call a function with less arguments than required without triggering an error as it will complete the argument values with undefined, so they didn't have this problem Makes sense? |
Right, I think that makes sense. Python actually wanting a proper set of arguments to a function does complicate the issue slightly. By the way, there's no tagged release or changelog I can find for 0.9 which did make this slightly more complicated to work out! I'll give it a go using |
Ok, I've got it working fine apart from relay mutations which need a bit of extra work I've done in the PR. It would also be very helpful to add some docs about |
Dangit, how was I all over the codebase yesterday and didn't see |
@syrusakbary is it possible to maybe have this as an option? We use the Especially the |
I would agree that |
I would like to discuss about that. Like: class Query(graphene.ObjectType):
say = graphene.String(what=graphene.String())
def resolve_say(self, what):
return "Hey {}".format(say) I think it's too verbose having Maybe using decorators (like I would like to get into a solution that could solve both points of view, or at least get closer. Merging More proposals? Thoughts? |
@syrusakbary yeah that is a quite difficult one. I mean the only other solution I could think of then is |
Ok, so this is a tricky design decision in some ways because we want to mimic the "argument magic" that javascript can manage in python. We've also got a possible name clash issue. Here are a few possible approaches:
|
Hi @mjtamlyn, @Globegitter. I've been researching and Jinja use the decorators for passing some extra args to the function. Also, if this is the chosen path for I'm not very sure yet about what's the good choice here though. Another option, is grouping context and info in other instance and pass something like: def resolve_blablabla(self, context_with_info, **args):
pass |
Maybe its coming from the Ruby world... but I kinda like the "magical" approach of automatically injecting values by argument name using This automatic objection could be implemented as a decorator too... (which can help resolving possible name by providing some sort of mapping between default name and var name...) |
@ekampf @mjtamlyn @Globegitter I've been thinking in a good way to solve this for a long time. If we use a magical approach using Also, the resolver in I'm thinking of merging back def resolve_blablabla(self, args, info):
context = info.context
# ... Thoughts? |
I think |
@syrusakbary Having thought about it a bit more I am leaning mostly towards the Also if the arguments passed in would ever change again it that approach would imo make it a bit simpler. That all said though, I do think your suggested approach also works. |
I think I found a way to make the resolvers much more fun to write using type annotations in Python 3. Would love to get some feedback! @Globegitter @mjtamlyn @ekampf |
According to the docs: http://docs.graphene-python.org/en/latest/types/objecttypes/
Followed by
As you can see, the example resolver method only has the |
Update tutorial-plain.rst
In case anyone is also having troubles on how to access the context, after an endless effort/search I found on this link: http://docs.graphene-python.org/projects/django/en/latest/authorization/ that I can access context like this:
Hope this helps anyone |
Hi @mjtamlyn . We're currently going through old issues that appear to have gone stale (ie. not updated in about the last 6 months) to try and clean up the issue tracker. If this is still important to you please comment and we'll re-open this. Thanks! |
The context story seems very confused in the newest version. My understanding from the graphql-js 0.5.0 release notes is that all resolving functions should now have the signature
([self,] args, context, info)
, as opposed to([self,] args, info)
.At the moment, if I update a
resolve_NAME
method to take context, then it throws argument errors.Also, mutations don't take context and therefore they cannot access context data. It think a patch similar to this one on graphql-relay-js is necessary for the relay mutations at least, but probably for all of them.
The text was updated successfully, but these errors were encountered: