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

Revise documentation #969

Merged
merged 4 commits into from
Jun 9, 2019

Conversation

dvndrsn
Copy link
Contributor

@dvndrsn dvndrsn commented May 19, 2019

  • Add missing reference to flask-graphql in integrations
  • align documentation for resolver arguments (use root for 1st argument
    instead of self)
  • explore use of parent instead of root for first argument
  • clarify resolvers and object type documentation
  • add documentation for Meta class options for ObjectType
  • expand quickstart documentation for first time users
  • streamline order of documentation for first time users (broad ->
    specific)
  • document resolver quirks

@dvndrsn
Copy link
Contributor Author

dvndrsn commented May 19, 2019

Tried to optimize for first time user experience by explaining some basic GraphQL concepts and putting more explicit linking between relevant parts of the documentation and the open source documentation. Documented some undocumented features for ObjectType Meta class.

There could be more work to be done to keep a light introduction to Graphene concepts that goes from Broad, High level concepts (ex. Schema) to more specific (ObjectType, then Fields). If we had an API reference then we could focus on making a really streamlined first time user experience and then document edge cases and specific API's in the API reference instead of the main body of documentation.

@dvndrsn
Copy link
Contributor Author

dvndrsn commented May 20, 2019

Resolver arguments are inconsistent in documentation - sometimes indicated as self and other times as root - however, there may be a better term to use for the first argument of a resolver.

I did some research in other libraries and documentation and found a couple of different terms that are used. I've listed them in an order that ranks them in how descriptive and accurate they are to describe this first resolver argument.

  • parent - this is used in the documentation for apollo server, pretty clear that the value is coming from the parent resolver's result.
  • root (or other descriptive argument name) - this is used in blog posts and in the graphql-js core examples, but I think there might be a misunderstanding - graphql-js example code only used root as the first argument name when the resolver is executed in a Root Query or Root Mutation. In other cases, the first argument of the the resolver is named to describe the value object that is being passed into the parameter (ex. character, planet, etc.). This is a good self-documenting approach in code, but may be confusing for documentation.
  • subject - this is used in the type implementation for graphql-js core, but a little abstract.
  • obj - used in graphql documentation for various libraries (including the other major Python GraphQL libraries), but is a little vague and an abbreviation (not ideal IMO).

In any event, self should probably not be used to describe this argument as resolvers never receive an instance of the class as the first argument of the resolver method. Especially in dynamic languages, naming for parameters is important as we often don't have type systems to tell us what the variable might be. Not naming the first parameter self not agree with linter rules sometimes, but using a different term is most appropriate here.

Let's try to align on one term to use across the documentation.

Copy link
Member

@jkimbo jkimbo left a comment

Choose a reason for hiding this comment

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

This is fantastic @dvndrsn ! Thank you so much! I've added some comments but let's get this merged in as soon as possible.

Regarding the naming for the "parent" argument for resolvers: my 2 cents is that it should be named after the object it's expecting to get (so person in the case of the Person type). I've written up more about it here: #921 (comment)

docs/quickstart.rst Outdated Show resolved Hide resolved
docs/types/objecttypes.rst Outdated Show resolved Hide resolved
Each field on an *ObjectType* in Graphene should have a corresponding resolver method to fetch data. This resolver method should match the field name. For example, in the ``Person`` type above, the ``full_name`` field is resolved by the method ``resolve_full_name``.

Each resolver method takes the parameters:
* :ref:`ResolverRootArgument` for the value object use to resolve most fields
Copy link
Member

Choose a reason for hiding this comment

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

ResolverParent? Since that is what you are calling it later. Also drop the Argument (or replace with Parameter) because the term arguments is also used to refer to the "arguments" to a field in the schema.

* This value object is then used as ``parent`` while calling ``Person.resolve_full_name`` to resolve the scalar String value "Luke Skywalker".
* The scalar value is serialized and sent back in the query response.

Each resolver returns the next :ref:`ResolverRootArgument` to be used in executing the following resolver in the chain. If the Field is a Scalar type, that value will be serialized and sent in the **Response**. Otherwise, while resolving Compound types like *ObjectType*, the value be passed forward as the next :ref:`ResolverRootArgument`.
Copy link
Member

Choose a reason for hiding this comment

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

👏 👏 👏

docs/types/objecttypes.rst Show resolved Hide resolved
@dvndrsn dvndrsn requested review from changeling and phalt as code owners June 9, 2019 13:56
dvndrsn added 3 commits June 9, 2019 10:59
- Add missing reference to `flask-graphql` in integrations
- align documentation for resolver arguments (use root for 1st argument
instead of self)
- explore use of `parent` instead of `root` for first argument
- clarify resolvers and object type documentation
- add documentation for Meta class options for ObjectType
- expand quickstart documentation for first time users
- streamline order of documentation for first time users (broad ->
specific)
- document resolver quirks
@dvndrsn dvndrsn force-pushed the clarify-resolvers-and-intro branch from 9d71a9e to a6ac7d4 Compare June 9, 2019 15:05
@dvndrsn dvndrsn requested a review from jkimbo June 9, 2019 15:13
@dvndrsn
Copy link
Contributor Author

dvndrsn commented Jun 9, 2019

@jkimbo, incorporated your suggestions.

@phalt @changeling - any feedback?

Copy link
Member

@jkimbo jkimbo left a comment

Choose a reason for hiding this comment

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

👍

@jkimbo jkimbo requested a review from ProjectCheshire June 9, 2019 22:40
@jkimbo jkimbo merged commit 5cb7d91 into graphql-python:master Jun 9, 2019
@jkimbo jkimbo mentioned this pull request Jun 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants