strawberry icon indicating copy to clipboard operation
strawberry copied to clipboard

Implicit description from class docstring

Open AadamZ5 opened this issue 4 years ago • 19 comments

Long story short, in my opinion,

This, (my class used for example)

...
@strawberry.type(description="Represents data about a device that is currently connected to the application")
class ActiveHdd(HddEntry):
    serial: ID
...

Should be equivalent to this,

...
@strawberry.type
class ActiveHdd(HddEntry):
    """
    Represents data about a device that is currently connected to the application
    """
    serial: ID
...

We can gather this from the __doc__ attribute on the class, or a clean sanitized string using getdoc from the inspect module,

from inspect import getdoc

class MyClass:
    """
    My class that should have a document string.
    """
    pass

assert getdoc(MyClass) == 'My class that should have a document string.'

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

  • We're using Polar.sh so you can upvote and help fund this issue.
  • We receive the funding once the issue is completed & confirmed by you.
  • Thank you in advance for helping prioritize & fund our backlog.
Fund with Polar

AadamZ5 avatar Jan 09 '21 07:01 AadamZ5

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 :)

patrick91 avatar Jan 09 '21 11:01 patrick91

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

Ambro17 avatar Jan 09 '21 14:01 Ambro17

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?

AadamZ5 avatar Jan 09 '21 20:01 AadamZ5

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

BryceBeagle avatar Jan 09 '21 20:01 BryceBeagle

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?

AadamZ5 avatar Jan 09 '21 20:01 AadamZ5

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.

BryceBeagle avatar Jan 09 '21 20:01 BryceBeagle

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 :)

patrick91 avatar Jan 09 '21 20:01 patrick91

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.

BryceBeagle avatar Jan 09 '21 20:01 BryceBeagle

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

aryaniyaps avatar Oct 07 '21 08:10 aryaniyaps

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

aryaniyaps avatar Oct 18 '21 05:10 aryaniyaps

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))

coady avatar Oct 22 '21 01:10 coady

@patrick91 @jkimbo is this proposal favored?

aryaniyaps avatar Oct 22 '21 04:10 aryaniyaps

@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.

jkimbo avatar Oct 22 '21 08:10 jkimbo

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.

coady avatar Nov 02 '21 04:11 coady

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.

aryaniyaps avatar Nov 02 '21 07:11 aryaniyaps

Workaround

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.

Implementation

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

Example

@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.

lonelyteapot avatar Sep 07 '22 13:09 lonelyteapot

Yet another workaround

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.

lukaspiatkowski avatar Nov 10 '23 10:11 lukaspiatkowski