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

[WIP] Initial work refactoring docs/ examples/ class Meta to class arguments, implementing explicit imports. #992

Open
wants to merge 1 commit into
base: next/master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions docs/execution/dataloader.rst
Original file line number Diff line number Diff line change
Expand Up @@ -106,10 +106,10 @@ leaner code and at most 4 database requests, and possibly fewer if there are cac

.. code:: python

class User(graphene.ObjectType):
name = graphene.String()
best_friend = graphene.Field(lambda: User)
friends = graphene.List(lambda: User)
class User(ObjectType):
name = String()
best_friend = Field(lambda: User)
friends = List(lambda: User)

def resolve_best_friend(self, info):
return user_loader.load(self.best_friend_id)
Expand Down
38 changes: 20 additions & 18 deletions docs/execution/execute.rst
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ For executing a query a schema, you can directly call the ``execute`` method on

.. code:: python

schema = graphene.Schema(...)
result = schema.execute('{ name }')
schema = Schema(...)
result = schema.execute("{ name }")

``result`` represents the result of execution. ``result.data`` is the result of executing the query, ``result.errors`` is ``None`` if no errors occurred, and is a non-empty list if an error occurred.

Expand All @@ -21,14 +21,15 @@ You can pass context to a query via ``context``.

.. code:: python

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

def resolve_name(root, info):
return info.context.get('name')
return info.context.get("name")

schema = graphene.Schema(Query)
result = schema.execute('{ name }', context={'name': 'Syrus'})

schema = Schema(Query)
result = schema.execute("{ name }", context={"name": "Syrus"})



Expand All @@ -40,22 +41,23 @@ You can pass variables to a query via ``variables``.

.. code:: python

class Query(graphene.ObjectType):
user = graphene.Field(User, id=graphene.ID(required=True))
class Query(ObjectType):
Copy link

Choose a reason for hiding this comment

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

Whenever I read docs examples like this I always ask "where the hell do I import that from though?".

I feel like if we remove the graphene bit we need to show people where it is imported from. Because copy/pasting this won't work.

Copy link
Author

Choose a reason for hiding this comment

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

This is something I've been meaning to bring up. I think we should, wherever possible, make snippets in the docs complete. That is, again as much as possible, allow them to be copy/pasted into a .py file and actually work. I echo your thought here.

Copy link
Member

Choose a reason for hiding this comment

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

+1 on including imports. I always get confused with that too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also importing a package would be a better approach rather than import each class separately. Makes reading code much easier if each external reference is easily identifiable.

user = Field(User, id=ID(required=True))

def resolve_user(root, info, id):
return get_user_by_id(id)

schema = graphene.Schema(Query)

schema = Schema(Query)
result = schema.execute(
'''
query getUser($id: ID) {
"""
query getUser($id: ID) {
user(id: $id) {
id
firstName
lastName
id
firstName
lastName
}
}
}
''',
variables={'id': 12},
""",
variables={"id": 12},
)
20 changes: 11 additions & 9 deletions docs/quickstart.rst
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ Let’s build a basic GraphQL schema from scratch.
Requirements
------------

- Python (2.7, 3.4, 3.5, 3.6, pypy)
- Python (3.6+, pypy)
Copy link
Contributor

Choose a reason for hiding this comment

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

3.5 should be supported too (until 3.8 is released in October).

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we might want to clarify the versions supported and the plan in ROADMAP.md

Copy link
Author

Choose a reason for hiding this comment

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

I went with 3.6+ after looking at what we're testing for in CI. This is absolutely open to discussion!

- Graphene (2.0)

Project setup
Expand All @@ -35,15 +35,17 @@ one field: ``hello`` and an input name. And when we query it, it should return `

.. code:: python

import graphene
from graphene import ObjectType, Schema, String
Copy link

Choose a reason for hiding this comment

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

This is what I mean about adding imports in docs - we should be doing it consistently.

Copy link
Author

Choose a reason for hiding this comment

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

Agreed. See above.

Copy link
Author

Choose a reason for hiding this comment

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

Is there a sphinx or other tool for actually embedding live code from, say, examples/, for inclusion inline in the docs?

Copy link
Contributor

@dvndrsn dvndrsn Jun 9, 2019

Choose a reason for hiding this comment

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

This might be what you're looking for @changeling literalinclude:

https://quick-sphinx-tutorial.readthedocs.io/en/latest/code.html

This option emphasize-lines is interesting too. Both are built into sphinx


class Query(graphene.ObjectType):
hello = graphene.String(argument=graphene.String(default_value="stranger"))

class Query(ObjectType):
hello = String(argument=String(default_value="stranger"))

def resolve_hello(self, info, argument):
return 'Hello ' + argument
return f"Hello {argument}"


schema = graphene.Schema(query=Query)
schema = Schema(query=Query)

Querying
--------
Expand All @@ -52,11 +54,11 @@ Then we can start querying our schema:

.. code:: python

result = schema.execute('{ hello }')
print(result.data['hello']) # "Hello stranger"
result = schema.execute("{ hello }")
print(result.data["hello"]) # "Hello stranger"

# or passing the argument in the query
result = schema.execute('{ hello (argument: "graph") }')
print(result.data['hello']) # "Hello graph"
print(result.data["hello"]) # "Hello graph"

Congrats! You got your first graphene schema working!
9 changes: 3 additions & 6 deletions docs/relay/connection.rst
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,9 @@ and ``other`` an extra field in the Connection Edge.

.. code:: python

class ShipConnection(Connection):
class ShipConnection(Connection, node=Ship):
extra = String()

class Meta:
node = Ship

class Edge:
other = String()

Expand All @@ -37,8 +34,8 @@ that implements ``Node`` will have a default Connection.

.. code:: python

class Faction(graphene.ObjectType):
name = graphene.String()
class Faction(ObjectType):
name = String()
ships = relay.ConnectionField(ShipConnection)

def resolve_ships(self, info):
Expand Down
23 changes: 10 additions & 13 deletions docs/relay/mutations.rst
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,12 @@ subclass of ``relay.ClientIDMutation``.
.. code:: python

class IntroduceShip(relay.ClientIDMutation):

class Input:
ship_name = graphene.String(required=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I like this better without graphene graphene graphene everywhere!

Did we have a discussion with folks to confirm that everyone is generally on the same page? The current convention is followed in all kinds of places so it will take some doing to change all the code examples.

Copy link
Member

Choose a reason for hiding this comment

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

I'm all for this. It makes the code much easier to read.

Copy link
Author

Choose a reason for hiding this comment

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

@dvndrsn We had a brief discussion on Slack about this. One reason for this PR as WIP is to further the conversation. See: https://graphenetools.slack.com/archives/CGR4VCHUL/p1559524058000700

@jkimbo I feel the same way.

faction_id = graphene.String(required=True)
ship_name = String(required=True)
faction_id = String(required=True)

ship = graphene.Field(Ship)
faction = graphene.Field(Faction)
ship = Field(Ship)
faction = Field(Faction)

@classmethod
def mutate_and_get_payload(cls, root, info, **input):
Expand All @@ -28,22 +27,20 @@ subclass of ``relay.ClientIDMutation``.
faction = get_faction(faction_id)
return IntroduceShip(ship=ship, faction=faction)



Accepting Files
---------------

Mutations can also accept files, that's how it will work with different integrations:

.. code:: python

class UploadFile(graphene.ClientIDMutation):
class Input:
pass
# nothing needed for uploading file
class UploadFile(ClientIDMutation):
class Input:
pass
# nothing needed for uploading file
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is unrelated to the changes that you did:

It feels weird to have example code with commented out bits. I'd prefer to have a viable example which uses comments to highlight the important things.

Copy link
Author

Choose a reason for hiding this comment

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

That's a good conversation to have. We need to start a doc/channel to discuss these things. One thought: Any code we use in the docs that mirrors actual code in the examples, IMHO, ought to be taken as is from the example code. What do you think?

Copy link
Author

Choose a reason for hiding this comment

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

Made a note in my local TODO of this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I really like this idea to extract most examples from a working API. I had a few ideas that I brainstormed in this issue:

#970


# your return fields
success = graphene.String()
# your return fields
success = String()

@classmethod
def mutate_and_get_payload(cls, root, info, **input):
Expand Down
26 changes: 10 additions & 16 deletions docs/relay/nodes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,10 @@ Example usage (taken from the `Starwars Relay example`_):

.. code:: python

class Ship(graphene.ObjectType):
'''A ship in the Star Wars saga'''
class Meta:
interfaces = (relay.Node, )
class Ship(ObjectType, interfaces=(relay.Node,)):
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you prefer this way of supplying meta args? I've not used it much myself since I'm used to Meta inner class with Django.

Copy link
Member

Choose a reason for hiding this comment

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

I've not used this way before either but it's quite nice. I guess since we're dropping Python 2 support we should just go all out on Python 3 features.

Copy link
Author

Choose a reason for hiding this comment

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

@dvndrsn I like this for graphene, as it feels more pythonic, with the Meta perhaps having it's place in graphene-django.

It's offered in UPGRADE-v2.0.md under Meta as Class arguments, which may highlight it as best practice to new users/upgrading folks.

Also, class Meta seems to offer some confusion among users, if SO is any indication:

https://stackoverflow.com/search?q=%5Bgraphene-python%5D+class+Meta

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that's true. Especially with Graphene-Django and Graphene-SQLAlchemy. Both of those projects have a lot of use of Meta parameters.

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like docs should at least present both approaches. As I tend to use Meta anyway in my Django project, where graphene-django is also used, it will be nice to have a clear indication that you can do that with graphene also. That may not be clear for folks less familiar with Python.

"""A ship in the Star Wars saga"""

name = graphene.String(description='The name of the ship.')
name = String(description="The name of the ship.")

@classmethod
def get_node(cls, info, id):
Expand All @@ -45,26 +43,22 @@ Example of a custom node:

.. code:: python

class CustomNode(Node):

class Meta:
name = 'Node'

class CustomNode(Node, name="Node"):
@staticmethod
def to_global_id(type, id):
return '{}:{}'.format(type, id)
return "{}:{}".format(type, id)

@staticmethod
def get_node_from_global_id(info, global_id, only_type=None):
type, id = global_id.split(':')
type, id = global_id.split(":")
if only_type:
# We assure that the node type that we want to retrieve
# is the same that was indicated in the field type
assert type == only_type._meta.name, 'Received not compatible node.'
assert type == only_type._meta.name, "Received not compatible node."

if type == 'User':
if type == "User":
return get_user(id)
elif type == 'Photo':
elif type == "Photo":
return get_photo(id)


Expand Down Expand Up @@ -94,7 +88,7 @@ Example usage:

.. code:: python

class Query(graphene.ObjectType):
class Query(ObjectType):
# Should be CustomNode.Field() if we want to use our custom Node
node = relay.Node.Field()

Expand Down
23 changes: 9 additions & 14 deletions docs/testing/index.rst
Original file line number Diff line number Diff line change
Expand Up @@ -40,14 +40,11 @@ To use the test client, instantiate ``graphene.test.Client`` and retrieve GraphQ

from graphene.test import Client


def test_hey():
client = Client(my_schema)
executed = client.execute('''{ hey }''')
assert executed == {
'data': {
'hey': 'hello!'
}
}
executed = client.execute("""{ hey }""")
assert executed == {"data": {"hey": "hello!"}}


Execute parameters
Expand All @@ -61,14 +58,11 @@ You can also add extra keyword arguments to the ``execute`` method, such as

from graphene.test import Client


def test_hey():
client = Client(my_schema)
executed = client.execute('''{ hey }''', context={'user': 'Peter'})
assert executed == {
'data': {
'hey': 'hello Peter!'
}
}
executed = client.execute("""{ hey }""", context={"user": "Peter"})
assert executed == {"data": {"hey": "hello Peter!"}}


Snapshot testing
Expand All @@ -95,7 +89,7 @@ Here is a simple example on how our tests will look if we use ``pytest``:
# This will create a snapshot dir and a snapshot file
# the first time the test is executed, with the response
# of the execution.
snapshot.assert_match(client.execute('''{ hey }'''))
snapshot.assert_match(client.execute("""{ hey }"""))


If we are using ``unittest``:
Expand All @@ -104,8 +98,9 @@ If we are using ``unittest``:

from snapshottest import TestCase


class APITestCase(TestCase):
def test_api_me(self):
"""Testing the API for /me"""
client = Client(my_schema)
self.assertMatchSnapshot(client.execute('''{ hey }'''))
self.assertMatchSnapshot(client.execute("""{ hey }"""))
14 changes: 8 additions & 6 deletions docs/types/abstracttypes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -20,18 +20,20 @@ plus the ones defined in ``UserFields``.

.. code:: python

import graphene
from graphene import AbstractType, ObjectType, InputObjectType, String

class UserFields(graphene.AbstractType):
name = graphene.String()

class User(graphene.ObjectType, UserFields):
pass
class UserFields(AbstractType):
name = String()


class UserInput(graphene.InputObjectType, UserFields):
class User(ObjectType, UserFields):
pass


class UserInput(InputObjectType, UserFields):
pass

.. code::

type User {
Expand Down
Loading