jmcarp / flask-apispec

MIT License
655 stars 156 forks source link

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

Closed mdantonio closed 4 years ago

mdantonio commented 4 years ago

Details for this PR are reported in #178

Basically:

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 commented 4 years ago

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%) :arrow_down:
flask_apispec/extension.py 100.00% <100.00%> (ø)
flask_apispec/utils.py 98.24% <100.00%> (-0.04%) :arrow_down:
flask_apispec/views.py 100.00% <100.00%> (ø)
flask_apispec/wrapper.py 98.55% <100.00%> (-0.07%) :arrow_down:

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 commented 4 years ago

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 commented 4 years ago

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: https://github.com/marshmallow-code/webargs/issues/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 commented 4 years ago

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.

mdantonio commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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