-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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
feat(api): bump marshmallow and FAB to version 3 #9964
Conversation
Codecov Report
@@ Coverage Diff @@
## master #9964 +/- ##
==========================================
+ Coverage 70.31% 70.90% +0.58%
==========================================
Files 594 404 -190
Lines 31608 13108 -18500
Branches 3221 3221
==========================================
- Hits 22225 9294 -12931
+ Misses 9275 3706 -5569
Partials 108 108
Continue to review full report at Codecov.
|
# Conflicts: # requirements.txt # superset/charts/api.py
Tagging this as 0.37 due to bugs it fixes in master + to make it easier to cherry pick for patch releases on 0.37 version. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few first pass comments. Looking forward to getting aboard {FAB,marshmallow}3! 👍
superset/charts/api.py
Outdated
query_context = ChartDataQueryContextSchema().load(json_body) | ||
except KeyError: | ||
return self.response_400(message="Request is incorrect") | ||
except ValidationError as err: | ||
return self.response_400( | ||
_("Request is incorrect: %(error)s", error=err.messages) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is except KeyError
still needed here? Based on the docs, load
only raises ValidationError
in 3.0+: https://marshmallow.readthedocs.io/en/stable/_modules/marshmallow/schema.html#Schema.load
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it seems weird, this may possible come from our custom post_load
https://github.com/apache/incubator-superset/blob/master/superset/charts/schemas.py#L736 and then from https://github.com/apache/incubator-superset/blob/master/superset/common/query_context.py#L57
superset/dashboards/api.py
Outdated
except ValidationError as err: | ||
return self.response_400(message=err.messages) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Being picky, but is it more pythonic to assign error
as opposed to err
? (Applies to other places, too)
tests/charts/schema_tests.py
Outdated
try: | ||
_ = ChartDataQueryContextSchema().load(payload) | ||
except ValidationError as errors: | ||
self.assertIn("row_limit", errors.messages["queries"][0]) | ||
self.assertIn("row_offset", errors.messages["queries"][0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this doesn't raise, won't the assertions just be skipped?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few small second pass comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would suggest bumping to latest patch of marshmallow, other than that LGTM 🎉
* feat(api): bump marshmallow and FAB to version 3 * revert query context tests changes * obey mypy * fix tests * ignore types that collide with marshmallow * preparing for RC2 * fix tests for marshmallow 3 * typing fixes for marshmallow * fix tests and black * fix tests * bump to RC3 and lint * Test RC4 * Final 3.0.0 * Address comments, fix tests, better naming, docs * fix test * couple of fixes, addressing comments * bumping marshmallow
SUMMARY
Bump Flask-AppBuilder to version 3 that implies marshmallow version 3 that is a major (breaking) release
Marshmallow upgrading to version 3: https://github.com/marshmallow-code/marshmallow/blob/31b784fc24ed637c82d87eac35d1a1bd58851f59/docs/upgrading.rst
FAB 3 changelog:
Improvements and Bug fixes on 3.0.0
BREAKING CHANGES:
swaggerview/v1
is now exposed onswagger/v1
Fixes: #10232
ADDITIONAL INFORMATION