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

Converged on webargs behavior for @use_kwargs and @use_args decorators #152

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rusnyder
Copy link

Description

WARNING: This PR contains a breaking change to the @use_kwargs decorator. I believe this is a good direction for this library to go, but proceed reviewing/merging with caution

This PR attempts to bring the @use_kwargs and @use_args decorators to be more inline with the inherited Flask webargs implementations by delegating directly to them instead of mimicking their behavior.

Changes:

  • Added @use_args decorator
    • Injects positional parameters from provided schema/mapping
    • Directly calls webargs.FlaskParser.use_kwargs under the hood, so behavior should exactly match Flask webargs
  • Modified @use_kwargs implementation to directly invoke webargs.FlaskParser.use_kwargs
    • Now only injects named parameters from provided schema/mapping
    • This will no-longer attempt to inject the kwargs mapping as the first argument (in absence of either named parameters or **kwargs in the view function)
    • Removes support for @use_kwargs(Schema(many=True)), as this is not supported by webargs and is now better supported by the @use_args decorator

Fixes

TODO

  • Fix build failures
  • Update documentation

raise Exception("@use_kwargs cannot be used with a with a "
"'many=True' schema, as it must deserialize "
"to a dict")
elif isinstance(schema, ma.Schema):
Copy link
Author

Choose a reason for hiding this comment

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

NOTE: What follows here is entirely to provide more informative error output, since this now shares the same failure cases as webargs which may break some existing code that uses flask-apispec. My thought here was to be as helpful as possible in transition, but knowing that this adds nothing functional, I'm not sure if the value add is worth it.

At the judgement of the maintainer, I'd be okay doing one of 3 things:

  1. Leaving this code as is
  2. Rewriting to only run on failure (i.e. - except on the expected failures and just provide a better error message)
  3. Remove this block entirely

@codecov-io
Copy link

Codecov Report

Merging #152 into master will increase coverage by 0.23%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #152      +/-   ##
==========================================
+ Coverage   97.68%   97.91%   +0.23%     
==========================================
  Files           8        8              
  Lines         345      383      +38     
==========================================
+ Hits          337      375      +38     
  Misses          8        8
Impacted Files Coverage Δ
flask_apispec/wrapper.py 98.86% <100%> (+0.61%) ⬆️
flask_apispec/apidoc.py 97.05% <100%> (ø) ⬆️
flask_apispec/__init__.py 100% <100%> (ø) ⬆️
flask_apispec/views.py 100% <100%> (ø) ⬆️
flask_apispec/annotations.py 91.66% <100%> (+1.42%) ⬆️

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 b51962a...4d1a8bf. Read the comment docs.

@paxnovem
Copy link
Contributor

I haven't reviewed the code fully, but I do agree with the idea of bringing flask-apispec's behavior in line with it's underlying libraries. I am not sure how breaking changes are handled for this project though. @sloria What are your thoughts on these changes?

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