Open AadamZ5 opened 3 years ago
Hi @AadamZ5! That's something we did at the very beginning but we removed this functionality because it was easy to forget about and expose implementation details (something that happened at work if I'm honest 😊).
What about using a new property on the class?
@strawberry.type
class ActiveHdd:
__description__ = """
Represents data about a device that is currently connected to the application
"""
serial: ID
Pinging @jkimbo, @BryceBeagle and @ulgens for more opinions on this :)
I like the new attribute a little more, since the docstring may be oriented to developers and leak some implementation details.
At work we had a similar discussion and agreed with the special attribute approach.
If you want to take the docstring as the field description you explicitly set __description__ = __doc__
and you get the desired behavior, without the risk of doing it accidentally
The extra attribute would work, but truly I see it as redundant to the description=
parameter in the decorator. The only reason I thought it would be a good idea was because it is a straightforward and implicit way to specify the description without actually specifying any explicit parameters or attributes. I love me some implicit-ness.
I think that if the description is gathered from __doc__
as a fallback, then we can avoid exposing any implementation details by explicitly setting description=
in the decorator. But again, you mention that it's easy to forget the implicit functionality, and I agree.
Maybe I should rethink how my class's docstring is being used. Perhaps I am not using it how it should be used. Regardless, this isn't a bug or issue.
What do you think?
I think that if the description is gathered from
__doc__
as a fallback
I disagree. You wouldn't want to potentially leak implementation details because you were unaware of a cool, but non-obvious, feature of the library. I think there should be a schema-wide toggle that enables the feature, but is disabled by default:
schema = Schema(...)
schema.config.docstring_descriptions = True
I think there should be a schema-wide toggle that enables the feature, but is disabled by default:
schema = Schema(...) schema.config.docstring_descriptions = True
I think this is a great idea @BryceBeagle. I thought of something along the lines of this, but wasn't sure of the best way to do it.
So since it seems like a lot of people use the docstring for implementation details, maybe I am using the docstring in an unconventional way. In other words, the docstring is probably not a suitable description. As I'm no employed or super experienced dev, what do you find the docstring is usually used for?
So since it seems like a lot of people use the docstring for implementation details, maybe I am using the docstring in an unconventional way. In other words, the docstring is probably not a suitable description. As I'm no employed or super experienced dev, what do you find the docstring is usually used for?
I can't speak for the other users here, but personally I don't aim to have implementation details in my docstrings. But I also don't actively avoid doing so. It would require a bit of extra scrutiny to make sure that all the items I'm referring to in the comment are, in fact, public members. Someone that's unaware of the feature may not even think about doing this.
So since it seems like a lot of people use the docstring for implementation details, maybe I am using the docstring in an unconventional way. In other words, the docstring is probably not a suitable description. As I'm no employed or super experienced dev, what do you find the docstring is usually used for?
That's a very good question, I think docstrings should be used as some sort of public documentation for classes, functions and modules, so ideally they'd work well for a GraphQL description. The problem is that it is too easy to do the wrong thing, personally I've seen docstrings used to explain how something was implemented (instead of how to use and when), I've done that too be honest.
As a aim for the library we should make it easy to devs to not do wrong things, but also give flexibility when needed. So we can think of adding a settings for this :)
I think docstrings should be used as some sort of public documentation for classes, functions and modules, so ideally they'd work well for a GraphQL description.
I guess it depends on your definition of "public". Usually when I write docstrings, I'm documenting things for coworkers/other developers, i.e. those with access to the source. Not for the customers/users of the product.
There's something to be said for in-source documentation for end users -- a lot of libraries do this (e.g. Qt), but not all. Those that do in-source documentation could definitely benefit from docstring-based documentation for their GraphQL schemas.
Documenting arguments has been hard in strawberry. Since they are just arguments passed to a function, why not document them using the function's docstring???
Here's what I had in mind.
@strawberry.field()
def my_field(self, info, my_arg: str) -> bool:
"""
my field description
:param my_arg: argument description
"""
pass
So since it seems like a lot of people use the docstring for implementation details, maybe I am using the docstring in an unconventional way. In other words, the docstring is probably not a suitable description. As I'm no employed or super experienced dev, what do you find the docstring is usually used for?
Just adding onto this, it is recommended to have a clean split between your business logic (service layer) and graphql resolvers. Therefore, IMO we shouldn't be putting implementation details in the resolver docstrings anyways.
this diagram is taken from: https://graphql.org/learn/thinking-in-graphs/#business-logic-layer
Here are some utilities to spark ideas. I've used this wrapper:
def doc_field(func: Optional[Callable] = None, **kwargs: str) -> StrawberryField:
if func is None:
return functools.partial(doc_field, **kwargs)
for name in kwargs:
argument = strawberry.argument(description=kwargs[name])
func.__annotations__[name] = Annotated[func.__annotations__[name], argument]
return strawberry.field(func, description=inspect.getdoc(func))
which requires passing in the arg descriptions explicitly: @doc_field(arg="description", ...)
. And now experimenting with one that parses the arguments too using a third-party lib:
def doc_field(func: Callable) -> StrawberryField:
docstring = docstring_parser.parse(func.__doc__)
annotations = func.__annotations__
for param in docstring.params:
argument = strawberry.argument(description=param.description)
annotations[param.arg_name] = Annotated[annotations[param.arg_name], argument]
docstring.meta = [] # strip params
return strawberry.field(func, description=docstring_parser.compose(docstring))
@patrick91 @jkimbo is this proposal favored?
@patrick91 @jkimbo is this proposal favored?
@aryaniyaps what proposal do you mean?
If you mean whether the class docstrings should be used as GraphQL descriptions: my opinion is that they should be (by default) because the Strawberry type is meant to map directly to the GraphQL type so it makes sense to me to take the class description and use it as the GraphQL description. The same should probably happen with field resolvers.
I also don't think it's a massive issue to expose implementation details in the description since it might be interesting for the client. If the concern is leaking that information publicly then introspection should probably be disabled completely.
If you're talking about your proposal to set field argument descriptions through the doc string then I'm not sure yet. It's a neat idea but it feels a bit magical and not necessarily what I would expect to happen. Also my concern is that there isn't a standard argument syntax in Python as far as I'm aware (for example the google style guide says you should document arguments different to your example: https://github.com/google/styleguide/blob/gh-pages/pyguide.md#383-functions-and-methods ) and I don't particularly want to have to maintain multiple ways of adding descriptions to arguments.
Personally I think we should just make this api work since it matches the field api:
@strawberry.field
def my_field(info, my_arg: str = strawberry.argument(description="my field description")) -> str:
...
Another thing to note @aryaniyaps is that you can attach other metadata to arguments like changing the name or marking it as deprecated. It's not clear to me how you would define that in the doc string.
Another issue to consider is whether the current way discourages documentation, or at least lengthy documentation. An advantage of docstrings is the builtin paragraph formatting. I'm curious how users handle line wrap and indentation using description=
. Even with the argument
⬆️ proposal, a long description would end up black-formatted as:
@strawberry.field
def my_field(
info,
my_arg: str = strawberry.argument(
description="my field description............................."
),
) -> str:
...
which seems less readable to me, particularly as the number of args grows.
even FastAPI does this to produce OpenAPI docs, if I'm not wrong, so I don't see any harm in having the same feature in strawberry. Both FastAPI routes and strawberry resolvers are meant to be lightweight transport layers, apart from the business logic anyways.
Here's a FastAPI route with a docstring based description:
from fastapi import FastAPI
app = FastAPI()
@app.get("/hello")
def send_hello():
"""
Sends an hello.
"""
pass
I would suggest that this feature should be enabled by default.
I implement it using a decorator. Note that it only works for types, but I think it can be easily tweaked to work with fields too.
from inspect import cleandoc
import strawberry.object_type
def load_description_from_docstring(cls):
type_definition = getattr(cls, "_type_definition", None)
if not isinstance(type_definition, strawberry.object_type.TypeDefinition):
raise TypeError(f"{cls} is not a Strawberry type")
raw_doc = cls.__doc__
if not isinstance(raw_doc, str):
raise ValueError(f"Invalid docstring for class {cls.__name__}")
type_definition.description = cleandoc(raw_doc)
return cls
@load_description_from_docstring
@strawberry.interface
class Node:
"""
An object with an ID.
"""
id: strawberry.ID
I use cleandoc
instead of getdoc
because the former may lead to unexpected behavior, i.e. inheriting description from ancestor classes.
I didn't like the decorator approach as it required marking every object explicitly with it. Instead I added this code just after schema creation:
def _apply_hack_for_strawberry_docstrings(schema: StrawberrySchema) -> None:
"""Force Strawberry to use the docstrings from the Python objects describing
GraphQL objects as GraphQL description.
Strawberry doesn't expose any configuration yet to do this, so we have to
use this hack to apply the docstrings manually.
Here is a link to the Strawberry issue tracking this:
https://github.com/strawberry-graphql/strawberry/issues/653
"""
# Iterate over all types in the schema
for gql_type in schema._schema.type_map.values():
strawberry_type = gql_type.extensions.get("strawberry-definition")
if strawberry_type is None:
# This is not a Strawberry type (e.g. a Graphene type)
continue
assert isinstance(strawberry_type, StrawberryType)
if gql_type.description is not None:
# The description has already been set, so we don't override it
continue
if isinstance(strawberry_type, StrawberryObjectDefinition):
origin = strawberry_type.origin
raw_doc = origin.__doc__
if not isinstance(raw_doc, str) or raw_doc.startswith(
f"{origin.__name__}("
):
# It seems that strawberry sets the docstring of a given Foo class
# to something that starts with "Foo(..." if the docstring is not
# present. Just skip it in that case assuming we don't have a
# docstring.
continue
elif isinstance(strawberry_type, EnumDefinition):
raw_doc = strawberry_type.wrapped_cls.__doc__
else:
raw_doc = None
if not isinstance(raw_doc, str):
# Make sure docstring is a string
continue
# Finally get the docstring as the GraphQL description
gql_type.description = inspect.cleandoc(raw_doc)
I am in the middle of transforming our codebase from using Graphene to Strawberry, so in order to preserve the behaviour of Graphene using the docstring by default this function did come in handly.
My view is that if support for native enums was merged then __doc__
should be used as type description. I would argue that people are familiar with this pattern from tools like Pydantic. The situation and expectations have changed over last couple of years.
Additionally most teams I worked with used pre-commit to generate and commit schema.gql
so it is reviewed before releasing.
(I guilty for not reading this discussion before submitting a PR)
Not sure where I wrote this, but I think we can do this, but as an opt-in only[1], so we don't change how the library works with current libraries.
I think the first implementation should support just the standard python docstrings (so on classes and methods), then we can think of this one:
class Node:
id: strawberry.ID
"""Doc for id"""
:)
[1] Maybe we can also have a default best practise configuration in future, where this and other options are enabled :)
Long story short, in my opinion,
This, (my class used for example)
Should be equivalent to this,
We can gather this from the
__doc__
attribute on the class, or a clean sanitized string usinggetdoc
from theinspect
module,I believe the description should be gathered from the docstring if no description parameter is explicitly set.
What do you think? I love the project so far! I hope to contribute more at some point.
Thanks!
Upvote & Fund