-
Notifications
You must be signed in to change notification settings - Fork 14k
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
chore: remove marshmallow-enum dependency and bump FAB #24499
Conversation
Codecov Report
@@ Coverage Diff @@
## master #24499 +/- ##
==========================================
- Coverage 69.03% 69.01% -0.02%
==========================================
Files 1901 1901
Lines 74006 74014 +8
Branches 8115 8116 +1
==========================================
- Hits 51088 51081 -7
- Misses 20807 20822 +15
Partials 2111 2111
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 12 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@@ -115,7 +115,7 @@ def test_default_missing_declaration_put_spec(self): | |||
self.assertEqual(rv.status_code, 200) | |||
response = json.loads(rv.data.decode("utf-8")) | |||
expected_mutation_spec = { | |||
"properties": {"id": {"format": "int32", "type": "integer"}}, |
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.
@dpgaspar is this a side-effect of how marshmallow-enum
was encoding the data?
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.
it's actually a change done on apispec: marshmallow-code/apispec#595
breaking change on 4.0.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.
@dpgaspar does that mean you're planning on holding off merging this until the next breaking release window?
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.
no, no, the 4.0.0 breaking change is on apispec, it's apispec==4.0.0, major 4 here is only a coincidence with future 4.0.0 on Superset
Nice! |
good point, but I don't think we need to pin mashmallow on |
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.
Very nice! I verified that the configuration_method
error that I was previously seeing on the codegen, where the enum values were missing, is now gone 👍 LGTM
Thank you for raising my attention to this issue, great work @villebro as always |
SUMMARY
Marshamallow and marshmallow-apispec support Enum types and since then marshmallow-enum as been archived.
This PR:
fields.Enum
onlyBEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION