Skip to content

Update object type docs #921

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

Merged
merged 2 commits into from
Mar 17, 2019
Merged

Update object type docs #921

merged 2 commits into from
Mar 17, 2019

Conversation

jkimbo
Copy link
Member

@jkimbo jkimbo commented Mar 16, 2019

Now that the default resolver handles both objects and dicts: #638

Now that the default resolver handles both objects and dicts
me = graphene.Field(Person)

def resolve_me(_, info):
def resolve_best_friend(_, info):
Copy link
Contributor

Choose a reason for hiding this comment

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

ahh I just opened an issue on your other PR about putting self into each method as per python convention, and also its used in other examples in the docs. But now here I see this place in the docs it is also set to be _ previously. Having it both ways in the docs feels not-consistent.

In the case of this particular PR your addition here matches the convention in this file of not having self so I'm fine with just merging this in and maybe we can figure out the whole self-vs-underscore thing across the docs separately...

Copy link
Member

Choose a reason for hiding this comment

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

And elsewhere it is root, info, **args

Consistent would be fantastic

Copy link
Member Author

Choose a reason for hiding this comment

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

@dan98765 so the reason I don't use self when defining resolvers is because self means a specific thing in python (the instance of the current class) but in Graphene the first argument to the resolver is not an instance of the current class. Actually resolvers work like static functions and the first param is whatever the parent resolver returned.

Using self in the past has led to people being confused about what the value actually is and assuming it's the current instance (which is not unreasonable) so I've taken to naming the value as _ when it's at the top of the tree (because if there is no root value then it's always None) or calling it a name that refers to what I'm expecting to receive. For example:

class User(graphene.ObjectType):
	name = graphene.String()

	def resolve_name(user, info):  # <-- the first argument will be a Django model instance
		return user.name

class Query(graphene.ObjectType):
	user_by_id = graphene.Field(User, id=graphene.Int())
	
	def resolve_user_by_id(_, info, id):  # <-- no root value so the first argument is always None
		return DjangoUser.objects.get(id=id)

I agree that we should standardise on something and actually thinking about it it makes sense to standardise on root for the top level Query resolvers (rather than _) so I'm happy to change the documentation to reflect that. I would also strongly recommend not using self for any resolver documentation because I think it ends up being confusing. Does that make sense @dan98765 @ProjectCheshire ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation, @jkimbo! I agree that we should not use self unless the it actually does represent the instance of the current class in the standard python way.

Actually resolvers work like static functions and the first param is whatever the parent resolver returned.

Could resolvers be decorated with @staticmethod to make this more explicit? Or does that break something?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm unsure if you can add @staticmethod to resolvers actually but I don't see why it wouldn't work. It would get quite tedious to have to do it everywhere though.

@ekampf ekampf merged commit 21cccf4 into master Mar 17, 2019
@ekampf ekampf deleted the update-object-type-docs-resolver branch March 17, 2019 19:43
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.

4 participants