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

Fix Marshmallow Schema's Meta exclude field error #431

Merged
merged 1 commit into from Apr 18, 2019

Conversation

@blagasz
Copy link

commented Apr 11, 2019

Mixing tuple and list when collecting fields in the Marshmallow extension may result in error.

@lafrech

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

Good.

Do you think you could test all combinations ([dump_only; exclude] x [tuple; list])?

Also, there are CI failures.

You should use a set, not a tuple, in the test, as the order doesn't matter (or perhaps better add ordered=True to the schema Meta).

(Note: no need to close/recreate a PR, you can amend/rebase and then push -f.)

@blagasz

This comment has been minimized.

Copy link
Author

commented Apr 12, 2019

@blagasz blagasz force-pushed the blagasz:dev branch 4 times, most recently from 2c1680b to 98fb51e Apr 12, 2019

@blagasz

This comment has been minimized.

Copy link
Author

commented Apr 12, 2019

pyupgrade and black fails but I don't really know from the log what their problem is.

@lafrech

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

Yes, the log is not verbose enough.

If you're using Python 3.6+ you can install pre-commit to do that locally before pushing.
https://github.com/marshmallow-code/apispec/blob/dev/CONTRIBUTING.rst

@blagasz blagasz force-pushed the blagasz:dev branch 2 times, most recently from 24b7091 to c634cb0 Apr 12, 2019

@lafrech

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

Sorry, I was not notified on the force-push.

I'd be tempted to factorize the tests using some obfuscating pytest parametrization trickery, but that's just personal taste/obsession. I can do that later on if I have spare time to waste.

I think this is good to merge.

Thanks.

@blagasz

This comment has been minimized.

Copy link
Author

commented Apr 18, 2019

@lafrech lafrech merged commit 9c12274 into marshmallow-code:dev Apr 18, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@lafrech

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

I did the rework and released in 1.2.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.