Skip to content
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

Add descriptions to fields and responses #786

Open
wants to merge 6 commits into
base: 1.21.x
Choose a base branch
from

Conversation

PaulWay
Copy link
Contributor

@PaulWay PaulWay commented Apr 8, 2022

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

…string

Signed-off-by: Paul Wayper <paulway@redhat.com>
Signed-off-by: Paul Wayper <paulway@redhat.com>
@JoelLefkowitz JoelLefkowitz changed the base branch from master to 1.21.x July 15, 2022 11:08
@JoelLefkowitz
Copy link
Collaborator

JoelLefkowitz commented Jul 17, 2022

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

@JoelLefkowitz JoelLefkowitz added review wanted Review wanted 1.21.x Release target in 1.21.x labels Jul 17, 2022
Copy link
Contributor

@onegreyonewhite onegreyonewhite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

type=openapi.TYPE_OBJECT,
properties=properties,
required=required or None
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -557,7 +592,7 @@ def field_to_swagger_object(self, field, swagger_object_type, use_references, **
if not isinstance(field, serializers.SerializerMethodField):
return NotHandled

method = getattr(field.parent, field.method_name, None)
method = getattr(field.parent, field.method_name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? Without default value this code will raise AttributeError.

description = getattr(field, 'description', getattr(field, '__doc__'))

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem like a good idea, because the Model is an internal entity and may contain help that may not be relevant to the API at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.22.x Release target in 1.22.x review wanted Review wanted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants