Add descriptions to fields and responses

[PaulWay]

This was added to drf-yasg2 a while back and didn't get backported into drf-yasg.

[JoelLefkowitz]

@PaulWay thanks for the time put into this. I've corrected the implementation and updated some of the test schema.

At the moment the unit tests are not functionally testing behaviour, rather they match the test output to a correct test schema. Since I've updated the 'correct' schema to include descriptions I'd like some additional reviewers to verify the changes before merging. More specifically, are the definitions that are now in tests/reference.yaml as users would expect them to be?

[onegreyonewhite]

@PaulWay good job! I think these changes will help the schema to be more understandable for developers.

[onegreyonewhite]

@PaulWay SwaggerType is, in fact, drf_yasg.openapi.Schema. Maybe it's better to insert description separately?

if description:
    result.description = description

Another way:

result = SwaggerType(
    ...
    description=description or None
)

[JoelLefkowitz]

@PaulWay I've cherry picked your two commits because the branch had diverged

[JoelLefkowitz]

Thanks for the suggestions on the PR @onegreyonewhite,

I've put the getattr(..., None) back and removed this section:

if description is None and hasattr(field, 'Meta') and hasattr(field.Meta, 'model') and hasattr(field.Meta.model, '__doc__'):
    description = field.Meta.model.__doc__

[PaulWay]

Hi @JoelLefkowitz - yeah, I was wading through some ugly git rebase work there and I'm sure I was getting it wrong. Cherry-picking those commits is a much better plan :-)

[PaulWay]

This is just my personal style, but I'd just return "" there rather than let it go through the line at 529...

[JoelLefkowitz]

@PaulWay that's a really interesting point. Is it because you think it's less readable or less performant?

I really like understanding peoples' different code style preferences. For me the priority is how easy it is to reason about the return types and having a single return statement makes it clearer at a glance. Although because this is a short function it's actually just as easy to spot that it returns a string either way:

if not doc:
    doc = ""

return " ".join(line.strip() for line in doc.splitlines() if line.strip())

if not doc:
    return ""

return " ".join(line.strip() for line in doc.splitlines() if line.strip())

I guess I'm just in the habit of only adding early returns for very long functions.

Another option we could have used is a ternary but I think most people dislike it:

return (
    " ".join(line.strip() for line in doc.splitlines() if line.strip())
    if doc
    else ""
)

We could also have designed the function to not accept null as a value and instead require the calling function to check the type.

For the performance considerations I generally don't take it as guaranteed that a line will be evaluated if it doesn't have any side effects. It looks like in python this is not true and very few lines are optimised away. Nevertheless without profiling for bottlenecks first I typically won't consider the performance impact of an extra line evaluation.

In terms of style for the repository, there isn't consistent approach followed in the repository so I will revert it back to the way you had it :)

[PaulWay]

Thanks Joel and it's definitely just a personal thing rather than trying to find any consistent style in the drf-yasg coding :-)

I use the early return idiom for situations where there's an obvious 'null case' like this, where no further processing needs to occur. It's mainly for readability, but it's also better performance for the null case. I tested this and the doc="" code runs in 0.37 seconds in the doc=None, whereas the return "" code runs in 0.04 seconds. Both cases run in 0.48 seconds when doc="Foo bar baz".

And stylistically it definitely wins over:

if not doc:
    return ""
else:
    return " ".join(line.strip() for line in doc.splitlines() if line.strip())

Why have the else and the indent there, indeed?

[PaulWay]

Your return type argument is a good one and for larger functions it would be more of a consideration.