Explain non-pythonic design and structures

[danpalmer]

This is quite a general thing, so I'd appreciate thoughts from the maintainers, and some ideas for solutions.

I come from a Python background, and while reading the documentation I've found that the code goes against things I would expect of Python code in various ways. While this isn't critical to being usable at a small scale, while thinking about how this will fit into our codebase, and how other developers will get up to speed on using it, I think it will hold back adoption significantly.

I'm sure there are good reasons for some of the unexpected designs, but I think it would be great to give time to this in the documentation.

Places where I think this documentation is necessary:

• What is a promise? Promises aren't a concept in Python, and it might be assumed that async/await could be used instead. Providing a brief reason for why promises are used in Graphene and links to further documentation would be great.

• self not being a class instance in resolver 'methods'. I don't understand the architectural reasons why this can't be the case, but in Python an instance method (like the resolver methods) is expected to take a self first argument. The documentation says that this isn't the case with Graphene (which is unexpected), but it still calls the argument self in some cases which further confuses the issue. It is mentioned that these methods act like static methods, however it's unexpected that they aren't decorated with the @staticmethod decorator.

• Interfaces are listed in a Meta class attribute. In normal Python I would expect these to be in the inheritance tree, perhaps as an abstract base class using abc. Again I'm sure there are great reasons why this is not the case, but giving some motivation for this in the documentation would go a long way to making Graphene easier to understand.

• Middleware is implemented object instances with a resolve method. Again I'm sure there are reasons for this, but I wonder if the class is needed, could middleware just be a function (I think this would be more Pythonic). Alternatively, middleware could be specified to Graphene as classes, not instances, and Graphene could instantiate on each request-response cycle so that the instance is able to hold data in the request lifecycle. This is how Django does it, and it can be useful with more complex middleware. Right now that is not well supported.

Graphene is powerful, and I'm confident that it will be a great way to implement GraphQL APIs in Python codebases, however right now I think it could be quite intimidating for Python developers looking at it for the first time because of how differently it appears to work. I believe most of this can be solved with documentation.

I'd appreciate your thoughts on the state of the documentation, your thoughts on the above examples that I've highlighted, and what you think the path forward is for documenting these things.

[syrusakbary]

Hi @danpalmer,

I think your concerns regarding the "pythonic" way of doing things are not just valid, but very reasoned and thoughtful.

Before adding into the docs, I would love to describe here the "whys" on each of the points you made, so we can go back and forth with the explanations until they are clear. And from there we can write them into the docs.

Promises

What is a promise? Promises aren't a concept in Python, and it might be assumed that async/await could be used instead. Providing a brief reason for why promises are used in Graphene and links to further documentation would be great.

You are completely right, with async/await syntax the promise abstraction is no longer needed (I even tweeted about it few months ago 😜).

The main reason Graphene/GraphQL-core was using Promises in the first place is for the need of a abstraction "future" to be managed in Python versions where the async/await syntax was not available (Python 2.7+, Python ~< 3.4).

It's also true that in Python and some of its libs the concept of Deferred and Future was also available, but there were some reasons for using a Promise abstraction instead:

1. It provides a "universal" way of working with futures/deferrals independently of the framework (gevent, tornado, threading, ...., as each of this implementation have a different implementation of a Future).
2. It provides a very easy API to chain this "future" values.
3. It incredibly simplified the GraphQL core codebase, making it easy for contributors to step on to the code: Custom scalars using numpy arrays fail is_nullish() test graphql-core#59

But the truth is, once Python 3 is used by most of the Graphene users, the promise abstraction can be safely removed :)

self in resolvers

self not being a class instance in resolver 'methods'. I don't understand the architectural reasons why this can't be the case, but in Python an instance method (like the resolver methods) is expected to take a self first argument. The documentation says that this isn't the case with Graphene (which is unexpected), but it still calls the argument self in some cases which further confuses the issue. It is mentioned that these methods act like static methods, however it's unexpected that they aren't decorated with the @staticmethod decorator.

At the beginning (in Graphene), all resolvers were receiving the args self, args, info, where self was actually an instance of the ObjectType itself.
So, for accessing the root value, the developer need to type: self.root. Also, for ease of development, a __getattr__ was created in the ObjectType so each time you do self.abc it was calling getattr(self.root, 'abc').

This resulted in very bad performance in the earlier versions of Graphene (previous to 1.0), as for each ObjectType resolution, the result needed to be wrapped in a new instance of the ObjectType.

So then, we have to solve other question also... why all resolvers are static without the need of them being decorated with @staticmethod?
The main reason for that is:

1. To simplify the code necessary to be written by the developer
2. If the developer forgets to wrap it with @staticmethod, there will be no real self (as we don't want to wrap the result because of performance as exposed before).

A very similar decision was made by the Python committee when the new __init_subclass__ method was introduced in PEP-487 choosing not to require a @classmehtod on the method, mainly because of minifying developer issues.

Eventually, both the Graphene types and the real datatypes will be merged into the same object (for static typing reasons, plus readability, I can explain this in another comment if necessary), so this hopefully will no longer be an issue.

Interfaces not inherited

Interfaces are listed in a Meta class attribute. In normal Python I would expect these to be in the inheritance tree, perhaps as an abstract base class using abc. Again I'm sure there are great reasons why this is not the case, but giving some motivation for this in the documentation would go a long way to making Graphene easier to understand.

In the first versions of Graphene (pre 1.0), the only way to inherit a specific Interface in a ObjectType was through Python object inheritance (rather than meta like right now, see the Graphene 1.0 upgrade guide)
However, the nature of ObjectTypes and Interfaces are mainly different (both hold fields, but one can be instantiated while the other don't...), mixing both classes to be inherited at the same time can result challenging:

1. When a class inherits an Interface, how can we check that is an ObjectType versus an abstract Interface? (like the Node interface).
2. How we can make both metaclasses not to collide (ObjectType metaclass and Interface metaclass).

Right now I believe this might be easier resolved thanks to the __init_subclass__ approach, but will wait until a valid (and easy to understand) proof of concept is made.

Middleware as a class rather than a function

Middleware is implemented object instances with a resolve method. Again I'm sure there are reasons for this, but I wonder if the class is needed, could middleware just be a function (I think this would be more Pythonic). Alternatively, middleware could be specified to Graphene as classes, not instances, and Graphene could instantiate on each request-response cycle so that the instance is able to hold data in the request lifecycle. This is how Django does it, and it can be useful with more complex middleware. Right now that is not well supported.

The reason Middleware was created as a class was because of it was created with the aim, of not only act in the resolution of a field, but also intercepting the beginning/end of a GraphQL query.
But the truth, is this was planned, but never added afterwards.

About middleware as just a function, it's also supported by Graphene, but sadly not documented.

---

Would love to hear your thoughts on this! :)

[KaySackey]

I'd say this was extremely helpful, and ought to be placed in an FAQ somewhere.

[nealmcb]

Thank you both!
Any updates on these issues a year later? Has this been incorporated into FAQs or documentation? Have the answers changed at all?

[stale]

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.