-
Notifications
You must be signed in to change notification settings - Fork 824
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
base: next/master
Are you sure you want to change the base?
Conversation
57bd959
to
9472e89
Compare
Hm.. This is gonna conflict quite a bit with my docs PR.. we need to start merging these faster.. 🤔 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good. Some questions and comments for discussion.
We should consider putting an item on docs backlog for adding doctest to the pipeline to ensure the examples are valid.
@@ -1,34 +1,27 @@ | |||
import graphene | |||
from graphene import ObjectType, Schema, Field, String | |||
from graphene import relay |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why keep this as a module import rather than importing relay
classes from graphene
as well (all are available in the top level module) or import the classes from graphene.relay
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to preserve this as a reminder to revisit this in conversation. Do we want to go completely explicit, or just top-level to highlight the sub-package. I'm waffling back and forth in my head on this.
class UploadFile(ClientIDMutation): | ||
class Input: | ||
pass | ||
# nothing needed for uploading file |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
class Input: | ||
ship_name = graphene.String(required=True) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@@ -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) |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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!
'''A ship in the Star Wars saga''' | ||
class Meta: | ||
interfaces = (relay.Node, ) | ||
class Ship(ObjectType, interfaces=(relay.Node,)): |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
ok = Boolean() | ||
person = Field(lambda: Person) | ||
|
||
def mutate(self, info, name): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def mutate(self, info, name): | |
def mutate(root, info, name): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The no-code done yet, but another good catch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the topic of what is the name of the first argument of a resolver called
is its own docs related discussion/issue. Its definitely not self
, so this is a good change IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Somewhere in the ecosystem, in an Issue
, there's a great discussion regarding root
as best, but for the life of me I can't find it again. If I do, I'll pop it into the docs tracking issue.
best_friend = graphene.Field(Person) | ||
class Query(ObjectType): | ||
me = Field(Person) | ||
best_friend = Field(Person) | ||
|
||
def resolve_me(_, info): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def resolve_me(_, info): | |
def resolve_me(root, info): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why some of these suggestions have removed a leading space, that wasn't what I was suggesting...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha. No worries. Even if I missed that, I'm trying to blacken the example code, so that would be fixed anyhow, I think.
ok = graphene.Boolean() | ||
person = graphene.Field(lambda: Person) | ||
ok = Boolean() | ||
person = Field(lambda: Person) | ||
|
||
def mutate(self, info, name): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def mutate(self, info, name): | |
def mutate(root, info, name): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is suggesting the root
change? Good catch, will fix on second pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a bunch of changes related to root
argument in this PR as well: #969
Please nitpick, comment, whatever all you want. That PR and this one are going to conflict in ways that will be annoying to resolve, I'm sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a thought, but perhaps we ahould look at the two, and consider which will make the most likely to conflict changes and stage them in order to minimize the conflicts? That is, if one makes minor syntactical changes and the other makes bigger changes to actual layout and wording...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we we can resolve the conflicts easily enough, but maybe if we can groom some stories around documentation there'll be less overlap.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that every method where root
or parent
is used as the first argument it should be decorated with @staticmethod
. Especially if that argument is not an instance of the class. Current approach is not pythonic.
human_by_name = graphene.Field(Human, name=graphene.String(required=True)) | ||
|
||
class Query(ObjectType): | ||
human_by_name = Field(Human, name=String(required=True)) | ||
|
||
def resolve_human_by_name(_, info, name): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def resolve_human_by_name(_, info, name): | |
def resolve_human_by_name(root, info, name): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See previous comments. Got it.
'''A ship in the Star Wars saga''' | ||
class Meta: | ||
interfaces = (relay.Node, ) | ||
class Ship(ObjectType, interfaces=(relay.Node,)): |
There was a problem hiding this comment.
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.
class Input: | ||
ship_name = graphene.String(required=True) |
There was a problem hiding this comment.
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.
Oh just saw this is WIP sorry @changeling . Feel free to ignore my comments and I can give it a proper review when the PR is actually ready. |
@jkimbo Not at all! That's exactly why I made this PR/WIP. To get some more eyes on. Thanks for the feedback and review. |
@@ -1,21 +1,24 @@ | |||
from graphene import InputObjectType, Float, ObjectType, String, Field, Schema | |||
|
|||
# Naming confusion/conflict with class Mutation and explicit import of graphene.mutation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd love some thoughts on this code. We're importing Mutation
from graphene
, previously used as graphene.Mutation
, and declaring class Mutation
.
Side note: We need to do a run through of all the doc examples in a functional context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's no good. This example will need to be improved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And that is exactly why you should just import graphene
so each external dependency is always clearly distinguishable as external.
07b3e47
to
65d799f
Compare
65d799f
to
ff988a6
Compare
Initial work refactoring class Meta to class arguments. Refactoring firehose imports to explicit imports. More blackening of example code. More refactoring of `class Meta` into class arguments.
ff988a6
to
eefcf40
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments around my usage of docs.
@@ -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): |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@@ -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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. See above.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
Comment on the use of graphene. graphene. graphene. everywhere. At first I thought this was annoying and didn’t understand why it was used in the docs but I’ve got back to importing graphene types this way because they have the same name as typing types, and we try to type everything we can as it is the way python is (thankfully) headed. On that note, is there a reason that this library doesn’t use the typing module? It would at least help new users like myself to figure out how to get things to work until this project starts moving more quickly on its various issues. For example root, context, info — what are these things? What are the possible values or uses for them? Sans proper documentation, at least knowing what types the authors intended those arguments to accept helps tremendously. |
@@ -32,17 +33,16 @@ needs to have the ``description`` property on it. | |||
|
|||
.. code:: python | |||
|
|||
class Episode(graphene.Enum): | |||
class Episode(Enum): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely need an import in this example for disambiguation.
class Episode(Enum): | |
from graphene import Enum | |
class Episode(Enum): |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
@changeling were you planning on updating this PR? |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
@changeling what state on 3.0? |
@@ -52,16 +52,16 @@ the ``Enum.from_enum`` function. | |||
|
|||
.. code:: python | |||
|
|||
graphene.Enum.from_enum(AlreadyExistingPyEnum) | |||
Enum.from_enum(AlreadyExistingPyEnum) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like mentioned in https://github.com/graphql-python/graphene/pull/992/files#r291840336 each use of class that also exists in standard lib should have import. Best if all examples have all imports required. I personally would keep import graphene
and graphene.xxx
as that is a good practice to import packages rather than members.
Refactoring firehose imports to explicit imports in docs/, examples/.
Some blackening of example code in docs/.
Refactoring of
class Meta
into class arguments.This is a first pass of docs/ and examples/ using explicit imports and refactoring
class Meta
into class arguments.Very little thought towards functionality of the documentation examples, just standardizing.