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

Custom docstring ignore tag #72

Merged
merged 4 commits into from
Sep 13, 2019
Merged

Custom docstring ignore tag #72

merged 4 commits into from
Sep 13, 2019

Conversation

dougthor42
Copy link
Contributor

@dougthor42 dougthor42 commented May 10, 2019

This allows the user to customize the docstring ignore separator.

The custom separator is set during blueprint initialization:

blp = Blueprint('foo', __name__, docstring_sep="~~~")
@blp.route('/', methods=['GET'])
def view_func():
    """Summary
    
    Descr
    ~~~
    This line and more ignored.
    """
    return "foo"

## Results in:
# summary: "Summary"
# description: "Descr"
blp = Blueprint('foo', __name__, docstring_sep=None)
@blp.route('/', methods=['GET'])
def view_func():
    """Summary
    
    Descr
    ~~~
    This line is included.
    """
    return "foo"

## Results in:
# summary: "Summary"
# description: "Descr\n~~~\nThis line is included."

Fixes #49.

@coveralls
Copy link

coveralls commented May 10, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling a965505 on dougthor42:custom-docstring-ignore-tag-gh49 into 5434725 on Nobatek:master.

flask_rest_api/utils.py Outdated Show resolved Hide resolved
@lafrech
Copy link
Member

lafrech commented May 10, 2019

Thanks for the PR.

Code looks good.

Regarding the test, I'd prefer a minimalist test that tests each case (explicit None, default "---" and custom string) on a dummy ad-hoc docstring. Real-life examples are nice but I don't think it is worth 100 lines of code in this case.

@dougthor42 dougthor42 changed the title [WIP] Custom docstring ignore tag Custom docstring ignore tag May 10, 2019
@@ -61,6 +61,7 @@ class Blueprint(
def __init__(self, *args, **kwargs):

self.description = kwargs.pop('description', '')
self.docstring_sep = kwargs.pop('docstring_sep', '---')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • Is this where you were thinking of adding the arg?
  • Where are the docs for the Blueprint so that I can add it there?

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking of making it a class attribute the user could override in a subclass, like HTTP_METHODS.

class Blueprint:

    DOCSTRING_SEP = '---'

I'd document this in docs/openapi.rst.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you still want me to move it to a class attribute rather than (or in addition to) an initialization arg?

Copy link
Member

Choose a reason for hiding this comment

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

Class attribute only would be consistent with the rest of the lib.

The separator will most probably be something the user will use consistently in all his Blueprints so subclassing and using a class attribute seems nice to me.

description is obviously different because it differs in all instances.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. I added it as a class attribute.

I also left the initialization arg so that users can do any of these:

# No need to subclass
blp0 = Blueprint('test', __name__, url_prefix='test', docstring_sep="~~~")

# But you can if you want to
class MyBlueprint(Blueprint):
    DOCSTRING_SEP = "~~~"

blp1 = MyBlueprint('test', __name__, url_prefix='test')

# and you can still override your subclass on a per-instance bases if desired.
blp2 = MyBlueprint('test', __name__, url_prefix='test', docstring_sep="^^^")

If that's not to your liking please let me know and I'll remove the init arg completely.

flask_rest_api/utils.py Outdated Show resolved Hide resolved
tests/test_blueprint.py Outdated Show resolved Hide resolved
tests/test_blueprint.py Outdated Show resolved Hide resolved
tests/test_utils.py Outdated Show resolved Hide resolved
@dougthor42
Copy link
Contributor Author

Regarding the test, I'd prefer a minimalist test that tests each case (explicit None, default "---" and custom string) on a dummy ad-hoc docstring. Real-life examples are nice but I don't think it is worth 100 lines of code in this case.

OK, I'll change that. Do you want to keep everything in that single test case with multiple asserts? Or would you rather a new test function for each? Eg:

def test_load_from_docstring_default_sep():
def test_load_from_docstring_custom_sep():
def test_load_from_docstring_none_sep():

Other questions:

  1. Do you have any preferences on squashing? If not, I'll rebase/squash this into 2 commits:
    1. the change to load_info_from_docstring and related tests
    2. the change to the Blueprint class and related tests
  2. Do you want the changelog update as part of this MR? If so, what would you like? (I don't see an "unreleased" section or anything like that.) Or will you handle the changelog update?
  3. Where else do docs need to be updated?

@@ -136,12 +139,12 @@ def store_method_docs(method, function):
for method in self.HTTP_METHODS:
if method in obj.methods:
func = getattr(obj, method.lower())
store_method_docs(method, func)
store_method_docs(method, func, sep=self.docstring_sep)
Copy link
Member

Choose a reason for hiding this comment

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

Can't you access self.docstring_sep in store_method_docs itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, at least not with the base I'm working from. store_method_docs is defined within Blueprint._store_endpoint_docs and does not accept the instance as an arg.

My current base is a1c2dfc, v0.15.0.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not familiar with closures. I thought self would be accessible since it is defined in the scope of Blueprint._store_endpoint_docs where store_method_docs is defined.

Currently out of office, I'll try to check this and the rest when I come back.

flask_rest_api/utils.py Outdated Show resolved Hide resolved
@lafrech
Copy link
Member

lafrech commented Jul 12, 2019

OK, I'll change that. Do you want to keep everything in that single test case with multiple asserts? Or would you rather a new test function for each?

I don't really mind. But thanks for asking.

Do you have any preferences on squashing? If not, I'll rebase/squash this into 2 commits:

  • the change to load_info_from_docstring and related tests
  • the change to the Blueprint class and related tests

That's perfect.

Do you want the changelog update as part of this MR? If so, what would you like? (I don't see an "unreleased" section or anything like that.) Or will you handle the changelog update?

Don't bother. I can do it.

I generally don't create an "unreleased" section. I just add the next version when needed.

Where else do docs need to be updated?

I think updating relevant section in docs/openapi.rst (https://github.com/Nobatek/flask-rest-api/blob/master/docs/openapi.rst#add-summary-and-description) is enough.

Thanks. Sorry for the delay...

@lafrech
Copy link
Member

lafrech commented Sep 13, 2019

Rebased, squashed (perhaps a little too much) and reworked. Let's merge.

@lafrech lafrech merged commit d5295d3 into marshmallow-code:master Sep 13, 2019
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.

Customizable docstring ignore tag
3 participants