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

Use TYPE_MAPPING from base_schema #72

Merged
merged 1 commit into from
Mar 19, 2020
Merged

Conversation

MartinAltmayer
Copy link
Contributor

I ran the tests with Python 3.6.10, 3.7.7, and 3.8.1. I also tested this branch in my application which uses Python 3.8.1.

Copy link
Owner

@lovasoa lovasoa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me ! Could you just add one line in the README about this new feature ? Maybe in the Customizing the base Schema section.

@lovasoa lovasoa merged commit 7794dd9 into lovasoa:master Mar 19, 2020
@MartinAltmayer
Copy link
Contributor Author

Thanks for merging! Shall I open a new PR for the README?

lovasoa added a commit that referenced this pull request Mar 19, 2020
@MartinAltmayer
Copy link
Contributor Author

Ok, I saw your own commit. Thanks for updating the README!

@lovasoa
Copy link
Owner

lovasoa commented Mar 19, 2020

Yes, I updated the README. Thank you for your contribution @MartinAltmayer ! I am going to update the changelog and publish a new release.

@lovasoa
Copy link
Owner

lovasoa commented Mar 19, 2020

Hmm... I'm thinking: my example may be super confusing.

class BaseSchema(marshmallow.Schema):
    TYPE_MAPPING = {CustomType: CustomField}

creates a schema where none of the default type mappings apply. This is probably never what anyone wants.

@lovasoa
Copy link
Owner

lovasoa commented Mar 19, 2020

Should we update our code to use {**base_schema.TYPE_MAPPING, **Schema.TYPE_MAPPING}, or leave it like that, but change the documentation and the tests ?

@lovasoa
Copy link
Owner

lovasoa commented Mar 19, 2020

I'd love to hear your opinion @sloria !

lovasoa added a commit that referenced this pull request Mar 19, 2020
@lovasoa
Copy link
Owner

lovasoa commented Mar 19, 2020

I implemented the first solution. I'll release it once I'm sure everyone agrees. @sloria , @MartinAltmayer ?

@MartinAltmayer
Copy link
Contributor Author

Looks fine to me, the user probably never wants to remove the default mapping.

@lovasoa
Copy link
Owner

lovasoa commented Mar 19, 2020

Ok, I'll publish that this weekend if no one opposes in the meantime.

@sloria
Copy link
Collaborator

sloria commented Mar 20, 2020

Yeah, the implemented behavior seems reasonable to me

@lovasoa
Copy link
Owner

lovasoa commented Mar 20, 2020

OK, it's deployed: https://pypi.org/project/marshmallow-dataclass/

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