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

Use regular function docstrings as a fallback route documentation #3

Closed
wants to merge 1 commit into from

Conversation

xalioth
Copy link

@xalioth xalioth commented Aug 1, 2018

If the user didn't specified a doc using @blp.doc(summary='Blah')
the function's doc is used as a fallback.

If the user didn't specified a doc using @blp.doc(summary='Blah')
the function's __doc__ is used as a fallback.
@coveralls
Copy link

coveralls commented Aug 1, 2018

Coverage Status

Coverage increased (+0.002%) to 99.58% when pulling 61fd224 on xalioth:betterdoc into 318807f on Nobatek:master.

@lafrech
Copy link
Member

lafrech commented Aug 2, 2018

Hi. Thanks for you interest and for the PRs. Don't expect too much reactivity, I'll be unavailable for a few days.

I generally don't like having two ways of doing the same thing, but I like this.

I'm tempted to go further. An operation can have both a summary and a description. This fits nice with usual docstrings:

def my_view(...):
"""Summary

Long description...
"""

We could use a parser that sets the content before the first empty line as summary, and the rest as description. And we could decide that a line starting with --- marks the end of the summary/description section and the rest is ignored.

Doing this would be compatible with apispec's docstring parsing (which parses the YAML docstring after the --- line). In fact, the whole summary/description parsing feature could be made available to apispec. Part of the needed code is in apispec already.

We'd use this parser to parse the docstrings for summary/description and the result would be overriden if those are specified using @blp.doc:

    def parser(func):
        [...]  # Black magic going on
        return {'summary': '...', 'description': '...'}


    def store_method_docs(method, function):
        doc = parser(function)
        doc.update(getattr(function, '_apidoc', {}))
        [...]

What do you think?

@xalioth
Copy link
Author

xalioth commented Aug 2, 2018

Sounds, good. I have the feeling however that this is exactly what the apispec FlaskPlugin should be doing, but it's not inside flask-rest-api

@lafrech
Copy link
Member

lafrech commented Aug 2, 2018

Because the plugin in apispec parses YAML stuff in the docstrings, which we don't use here. All web framework plugins in apispec are YAML-oriented. We don't need that because we take parameters/responses information from the Blueprint decorators.

Thinking of it, maybe part of what we do in the Flask plugin here could be added to the one in apispec.

@xalioth
Copy link
Author

xalioth commented Aug 2, 2018

Yes there is a kind of conflict with the auto parameter/response schema stuff, I think it's fine to stick to regular docstring for this framework.

Though I would love being able to document my Schema fields, and error responses

@lafrech
Copy link
Member

lafrech commented Aug 2, 2018

Though I would love being able to document my Schema fields

Not sure what you mean, here. Thanks to MarshmallowPlugin, it should be automatic. You can add description='Some string' to any field to add a description. And you should be able to add any other such attribute that exists in the OpenAPI spec the same way.

    nickname = fields.String(description='Nickname or surname', 'deprecated'=True)

and error responses

I think we did it in our app with a bit of hacking. The framework sure doesn't help. This is something we would like to improve. It depends on marshmallow-code/apispec#245. Feel free to comment there if you have ideas.

Back to the subject of this PR, I'm realizing that the summary/description use case shown in the example works for MethodView but for normal view functions, I guess you'd have to pass a method/description mapping (assuming it works, I didn't check):

                @blp.doc(get={'summary': 'Find pets by ID', 'description': ''}, post={...})
                def view_function(...):

(Note to self: fix Blueprint.doc docstring. The method expects kwargs, not dict. In its current form, it is a syntax error.)

I think your proposal is well suited for MethodView but in the case of a multi-method function, I'm not sure how to use the summary/description in the docstring. Or am I missing something?

(We mostly use MethodView as shown in the docs and examples. We use classic view functions for specific stuff, and we try to support them through the tests as much as possible. I don't think we ever used a multi-method view function.)

@xalioth
Copy link
Author

xalioth commented Aug 2, 2018 via email

@lafrech
Copy link
Member

lafrech commented Aug 2, 2018

I use marshmallow schema created from MongoEngine schemas and converted with marshmallow-mongoengine

You could be interested in µmongo (same author as marshmallow-mongoengine). It uses marshmallow internally and IIRC it passes those extra-parameters to generated marshmallow schemas.

Right, maybe we should just adopt yaml doc style like in APISpec flask plugin, but override it with the Schema for request/response and whatever is passed with @blp.doc

Yeah, maybe, but I really wish we could stay clear of YAML docstrings.

@lafrech
Copy link
Member

lafrech commented Aug 20, 2018

I'm realizing that the summary/description use case shown in the example works for MethodView but for normal view functions, I guess you'd have to pass a method/description mapping

I just checked and this is wrong. When Blueprint.doc is called on a view function, all documentation information is duplicated for every method this function answers to. I just modified the tests to make that clearer. And I fixed the example in the docstring.

The limitation is that when a function answers to multiple methods, the documentation is the same for all. I don't really see a practical case where one would have to use a function with multiple methods when designing a rest API. It generally ends up with multiple if/else cases in the function, and in the end I find it clearer to use different functions. Overall, this limitation does not bother me.

As a consequence, I think the docstring approach is fine after all, with no YAML needed.

@lafrech
Copy link
Member

lafrech commented Aug 21, 2018

@xalioth, I gave it a shot. What do you think of #5?

@xalioth
Copy link
Author

xalioth commented Aug 23, 2018 via email

@lafrech
Copy link
Member

lafrech commented Aug 23, 2018

Implemented in #5.

@lafrech lafrech closed this Aug 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants