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 type subclass check #102

Merged
merged 1 commit into from
Jan 1, 2020

Conversation

atmo
Copy link
Contributor

@atmo atmo commented Dec 28, 2019

Hi @fuhrysteve ! #81 along with #86 introduced a bug that causes builds to fail. This PR fixes it.

What happened

#81 added sequential subclass check that iterated over the mapping from marshmallow types to python types. #86 added fields.Number as a key and decimal.Decimal as a corresponding value. Since only Python 3.6 introduced ordered dictionaries, in previous versions order was not guaranteed.

This means that sometimes the keys in that dictionary were ordered in such a way that fields.Number came first. That meant that python type corresponding to fields.Integer became decimal.Decimal and not int as expected. As JSON schema output for Decimal and int differs too, that lead to failing tests.

How it was fixed

Instead of dict I iterate over a tuple of tuples - this will guarantee the order of subclass checks. I also reworked the mapping and handcrafted it into the library so that we don't depend on what the order was in marshmallow TYPE_MAPPING dictionary: note that we couldn't, for example, reason about corresponding Python type for fields.Raw as it could be either list, set or tuple.

@coveralls
Copy link

Coverage Status

Coverage remained the same at ?% when pulling 2479989 on atmo:fix_marshmallow_type_check into 3c579c7 on fuhrysteve:master.

@fuhrysteve
Copy link
Owner

Thanks for the pull!!!!

@fuhrysteve fuhrysteve merged commit 81cc98d into fuhrysteve:master Jan 1, 2020
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.

3 participants