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

Bump to webargs 6.0+; drop python 2 and marshmallow 2 compatibility #195

Merged
merged 7 commits into from
Aug 28, 2020

Conversation

mdantonio
Copy link
Contributor

@mdantonio mdantonio commented Jul 7, 2020

Details for this PR are reported in #178

Basically:

  • changed the locations parameter in use_kwargs to reflect changes in webargs 6.0.0+ => locations:tuple -> location:str

  • Fixed tests to add a location when missing (due to default location changed in webargs, now an explicit location is required more than before)

  • Dropped compatibility with python2 (removed six, conditional requirements in setup, replaced Classifiers with python_requires, removed # -- coding: utf-8 --, and other small changes).

Open issue: I'm unable to make 4 tests to work, all have multiple use_kwargs. At the moment these tests are commented in test_views.py (Added a "# I'M UNABLE TO MAKE THIS WORK" comment to mark them) to let other tests on complete.

@codecov-commenter
Copy link

codecov-commenter commented Jul 7, 2020

Codecov Report

Merging #195 into master will decrease coverage by 0.31%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #195      +/-   ##
==========================================
- Coverage   97.58%   97.26%   -0.32%     
==========================================
  Files           8        8              
  Lines         372      366       -6     
==========================================
- Hits          363      356       -7     
- Misses          9       10       +1     
Impacted Files Coverage Δ
flask_apispec/__init__.py 100.00% <ø> (ø)
flask_apispec/paths.py 100.00% <ø> (ø)
flask_apispec/annotations.py 90.24% <100.00%> (ø)
flask_apispec/apidoc.py 94.87% <100.00%> (-1.34%) ⬇️
flask_apispec/extension.py 100.00% <100.00%> (ø)
flask_apispec/utils.py 98.24% <100.00%> (-0.04%) ⬇️
flask_apispec/views.py 100.00% <100.00%> (ø)
flask_apispec/wrapper.py 98.55% <100.00%> (-0.07%) ⬇️

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 975658c...7697fd7. Read the comment docs.

@sloria
Copy link
Collaborator

sloria commented Jul 7, 2020

Thanks! Looks like a great start. I probably won't be able to take a deep look at this until the weekend, but others using this project should feel welcome to review this

@mdantonio
Copy link
Contributor Author

Thanks! Looks like a great start. I probably won't be able to take a deep look at this until the weekend, but others using this project should feel welcome to review this

Hello @sloria had you the opportunity to review the PR?

Open issue: I'm unable to make 4 tests to work, all have multiple use_kwargs. At the moment these tests are commented in test_views.py (Added a "# I'M UNABLE TO MAKE THIS WORK" comment to mark them) to let other tests on complete.

I was thinking again about the 4 tests that I'm unable to make work... Since they are all multiple use_kwargs che motivation is here: marshmallow-code/webargs#267

Now unknown fields are raised by default and so all multiple schemas on the same location will start to fail.
By setting the schemas with unknown=EXCLUDE everything works as expected

To make a quick test I changed this:

        @use_kwargs({'name': fields.Str()}, location='querystring')
        @use_kwargs({'instrument': fields.Str()}, location='querystring')

into this:

class NameSchema(Schema):
    name = fields.Str()
    class Meta:
        unknown = EXCLUDE


class InstrumentSchema(Schema):
    instrument = fields.Str()
    class Meta:
        unknown = INCLUDE

[...]
        @use_kwargs(NameSchema, location='querystring')
        @use_kwargs(InstrumentSchema, location='querystring')

Is there a way to directly define the unknown as decorator parameter? i.e. something like:

        @use_kwargs({'name': fields.Str()}, location='querystring', unknown=EXCLUDE)
        @use_kwargs({'instrument': fields.Str()}, location='querystring', unknown=EXCLUDE)

I may change the tests by setting all multiple schemas with unknown=EXCLUDE to make all tests to work but maybe you want to add more tests to cover all possible combinations of multiple schemas. Two schemas should success if both set with unknown=EXCLUDE (or INCLUDE) or if set on different locations. All other cases should fail

@sloria
Copy link
Collaborator

sloria commented Aug 6, 2020

Sorry I haven't had a chance to look at this more closely yet. Thank you for continuing to work on it.

Is there a way to directly define the unknown as decorator parameter?

No, it needs to be set on the schema.

I think your plan to set unknown=EXCLUDE on the schemas for those specific cases is fine for the time being.

Inputs converted from dict to schemas with Meta.unknown=EXCLUDE
@mdantonio
Copy link
Contributor Author

Ok, it is done. Re-included the 4 tests on multiple schemas

Now all tests are running.

Let me know if I can contribute with something more

@mdantonio
Copy link
Contributor Author

Ops, tests fail on Marshmallow 2.13.0, due to this import:

from marshmallow import EXCLUDE

We have to bump the minimum marshmallow version from 2.0.0 to something more... May you suggest me the correct version? Maybe 3.0.0?

@mdantonio
Copy link
Contributor Author

Tried with several 2.x versions (up to 2.21.0) and none of them work

marshmallow 3.0.0 works fine

-> changed in both setup.py (from >=2.0.0 to >=3.0.0) and travis (from 2.13.0 to 3.0.0)

@sloria
Copy link
Collaborator

sloria commented Aug 6, 2020

Yeah, I think bumping marshmallow to ~3.0 makes sense. marshmallow 2 will be EOL in 2 weeks anyway.

@sloria sloria changed the title [WIP] Bump to webargs 6.0+ and drop of python2 compatibility Bump to webargs 6.0+; drop python 2 and marshmallow 2 compatibility Aug 28, 2020
@sloria sloria merged commit 6dfbd3f into jmcarp:master Aug 28, 2020
jollysahil pushed a commit to levrofin/flask-apispec that referenced this pull request Sep 12, 2023
…mcarp#195)

* Compatibility fix to bump webargs requirement to 6.0+
Breaking change: in use_kwargs replaced locations:tuple with location:str

* Dropped compatibility with python 2

* Bug fix

* Restored tests on multiple schemas
Inputs converted from dict to schemas with Meta.unknown=EXCLUDE

* Bump marshmallow minimum version from 2.0.0 to 3.0.0

* Added a comment to explain the introduction of schemas in unknown = EXCLUDE used during tests

* Run pyupgrade

Co-authored-by: Steven Loria <sloria1@gmail.com>
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.

None yet

3 participants