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

fields.Function should support func=None #325

Closed
DamianHeard opened this Issue Nov 8, 2015 · 5 comments

Comments

Projects
None yet
3 participants
@DamianHeard
Contributor

DamianHeard commented Nov 8, 2015

I see the fields.Function being pretty useful with in webargs which presently only deals with deserialization. Unfortunately at the moment I have to provide a dummy function for the func argument or I'll hit an exception. It's entirely possible that there's a better field type within Marshmallow for addressing this but I don't think so.

I have a bit of a question mark around whether it makes sense to maintain backward compatibility and leave the signature as is:

class Function(object):
    def __init__(self, func, deserialize=None, **kwargs):
        # func is allowed to be None
        ...

or shift to something like

class Function(object):
    def __init__(self, serialize=None, deserialize=None, **kwargs):
        ...

which obviously isn't backward compatible but provides a clearer interface. Also, there's a bit of a question mark around whether the class should raise an exception if both serialize and deserialize are missing or whether it should just continue on despite being useless - probably an exception makes sense.

Happy to provide a PR should this change be acceptable.

@sloria

This comment has been minimized.

Member

sloria commented Nov 8, 2015

I am open to this. We need to maintain backwards compatibility with the func argument, though I would be open to deprecating func in favor of serialize (func would be fully removed in marshmallow 3.0).

here's a bit of a question mark around whether the class should raise an exception if both serialize and deserialize are missing or whether it should just continue on despite being useless - probably an exception makes sense.

Yes, I think it makes sense for an exception to be raised if both are omitted.

On a related note: I think it would make sense to set dump_only and load_only based on func/serialize and deserialize, respectively.

@sloria sloria added the enhancement label Nov 8, 2015

@justanr

This comment has been minimized.

Contributor

justanr commented Nov 9, 2015

If func and serialize are both provided during the deprecation period, which would be preferred?

@sloria

This comment has been minimized.

Member

sloria commented Nov 9, 2015

serialize would be preferred.

@sloria

This comment has been minimized.

Member

sloria commented Nov 11, 2015

Closed by #327

@sloria

This comment has been minimized.

Member

sloria commented Nov 11, 2015

The Method field should be made consistent with Function. I've opened an issue here: #329

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment