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

Added new parameter in Route decorator 'hidden_from_api_spec'; #277

Closed
wants to merge 2 commits into from
Closed

Conversation

0x78f1935
Copy link

  • Added new parameter in Route decorator 'hidden_from_api_spec'
  • Added related unittest
  • Added missing required libraries for unittests in dev-requirements.txt
  • Updated AUTHORS.rst

See #276

I never squashed myself and i Kinda screwed up the previous pr.
Re-forked the original library and applied the changes.

@lafrech Thank you for taking the time for those changes!

On another note, I've found some alternatives for ReDoc which I would like to implement. I might create a new PR for this if you are oke with this. See https://stackoverflow.com/questions/36634281/list-of-swagger-ui-alternatives

@codecov
Copy link

codecov bot commented Sep 10, 2021

Codecov Report

Merging #277 (e59fde5) into master (f8cdb5e) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head e59fde5 differs from pull request most recent head 7fc1d66. Consider uploading reports for the commit 7fc1d66 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master     #277   +/-   ##
=======================================
  Coverage   99.74%   99.74%           
=======================================
  Files          14       14           
  Lines         791      792    +1     
  Branches      144      145    +1     
=======================================
+ Hits          789      790    +1     
  Partials        2        2           
Impacted Files Coverage Δ
flask_smorest/blueprint.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f8cdb5e...7fc1d66. Read the comment docs.

@0x78f1935
Copy link
Author

@lafrech Any updates on this? Thanks in advance

Copy link
Member

@lafrech lafrech left a comment

Choose a reason for hiding this comment

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

Looks good. Just minor comments about wordings.


I don't think we need to modify dev-requirements. What I do is

pip install -e .
pip install -r dev-requirements

I shall pin the dependencies someday and activate dependabot, but I'll do it in another PR and with strict equalities.

tests/test_blueprint.py Outdated Show resolved Hide resolved
@0x78f1935 0x78f1935 requested a review from lafrech September 16, 2021 14:11
Copy link
Member

@lafrech lafrech left a comment

Choose a reason for hiding this comment

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

Please undo the dev-requirements.txt change.

git reset HEAD~1
git co dev-requirements.txt
git add flask_smorest tests
git commit -m "Add hidden_from_api_spec parameter to route/add_url_rule"
git add . AUTHORS.rst
git commit -m "Add 0x78f1935 to AUTHORS.rst"
git push -f

@0x78f1935 0x78f1935 requested a review from lafrech September 16, 2021 22:35
@0x78f1935
Copy link
Author

0x78f1935 commented Sep 16, 2021

Uhm before approving, perhaps it is better to change hidden_from_api_spec from boolean to a list.
An empty list represents False (default value).

The list itself can contain the following responses ['GET', 'POST', 'UPDATE', 'PUT', 'DELETE', etc etc].
Which should hide only the methods listed in hidden_from_api_spec.

This way you can hide endpoints such as
@blp.route('/auth/oauth2/authorize', methods=['GET', 'POST'], hidden_from_api_spec=['GET'])

What do you think about this proposal?

@0x78f1935
Copy link
Author

I'm sorry but it takes pretty long before obtaining any answers, I really could use this feature. @lafrech any updates?

@lafrech
Copy link
Member

lafrech commented Sep 27, 2021

I'm reluctant to add API surface for a feature that I believe is a corner case. I'd be happier if this was easily achievable in user code. Unfortunately, I can't find a way to do that.

It would be easy if we could pass arbitrary kwargs to werkzeug's add_url_rule. Then the user would just have to override _store_endpoint_docs and get hidden_from_api_spec from **options.

Since this is not feasible, I'm willing to add the feature to the core. I'd like to keep it simple but the method list is a legit case too and also wouldn't be easy to achieve without duplicating too much code.

You may propose an implementation.

hidden_from_api_spec may be False (default), True (hide all methods) or a list of methods to hide. (Don't use [] as default...)

Pass hidden_from_api_spec to _store_endpoint_docs and in there, at the beginning, use it to filter self.HTTP_METHODS (keep the order).


Regarding answer delays, please realize that most of the maintenance work in this organization is benevolent work. Adding features we need and keeping the lib up to date is part of my work, but working on corner-case features we won't be using is not and at the end of the day, I end up doing maintenance chores on my free time. I'm not complaining, just saying I can't guarantee answer delays. In this context, bug fixes get higher priority than such features.

It also takes time to answer because before letting someone code a PR, I try to be sure they are on the right track to avoid wasting their time.

Lastly, an important part of the work to have this lib running happens in webargs, apispec and marshmallow itself and may not be visible here.

If you really need a feature, you may override or fork until it gets added. Pinging maintainers generally doesn't help.

@0x78f1935
Copy link
Author

I'm reluctant to add API surface for a feature that I believe is a corner case.

This makes total sense. I get why.

If you really need a feature, you may override or fork until it gets added. Pinging maintainers generally doesn't help.

It's kinda that the "higher hierarchy" expected me to go after this PR since we are about to use flask_smorest in our production environment. I'm sorry if i came over as a little pushy. This is not my intention. I can make a workaround, so don't worry about it. The thing is that I end up with messy unnecessary duplicated code just like you described 😅 .

Thank you for your response, i can forward this to my project leader.


I'm almost done with finalizing our API, There are a few endpoints I would like to hide so I will come back to this.

hidden_from_api_spec may be False (default), True (hide all methods) or a list of methods to hide. (Don't use [] as default...)

Pass hidden_from_api_spec to _store_endpoint_docs and in there, at the beginning, use it to filter self.HTTP_METHODS (keep the order).

I see what I can do, but those instructions are very clear. I let you know!

This pull request was closed.
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.

2 participants