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

Support passing specific fields to `partial` #369

Merged
merged 1 commit into from Jan 13, 2016

Conversation

Projects
None yet
4 participants
@tdevelioglu

tdevelioglu commented Dec 28, 2015

Support passing specific fields to partial. This restricts the fields
allowed to be missing during partial deserialization to those specified
in the value of partial.
This is particularly useful in api's when dealing with non client-generated
primary key fields (at creation), where only a specific field is allowed
to be missing, and you'd still like to use the same schema.

@sloria

View changes

marshmallow/marshalling.py Outdated
if partial:
# Ignore missing field if we're allowed to.
if partial is True or\
is_collection(partial) and attr_name in partial:

This comment has been minimized.

@sloria

sloria Dec 29, 2015

Member

We can avoid type-checking here if the default for partial is changed to tuple().

This comment has been minimized.

@tdevelioglu

tdevelioglu Dec 29, 2015

It breaks the tests, on False values, though

This comment has been minimized.

@sloria

sloria Jan 11, 2016

Member

Ah, fair enough. This is fine as is.

This comment has been minimized.

@sloria

sloria Jan 13, 2016

Member

Actually, we can at least avoid type-checking within the for loop:

partial_is_collection = is_collection(partial)
for attr_name, field_obj in iteritems(fields_dict):
 ...
    if (
        partial is True or
        (partial_is_collection and attr_name in partial)
    )

This comment has been minimized.

@tdevelioglu

tdevelioglu Jan 13, 2016

good one, I've updated it.

@sloria

View changes

marshmallow/schema.py Outdated
partial = self.partial if partial is None else bool(partial)
if partial is None:
partial = self.partial
elif not utils.is_collection(partial):

This comment has been minimized.

@sloria

sloria Dec 29, 2015

Member

Is this branch necessary?

This comment has been minimized.

@tdevelioglu

tdevelioglu Dec 29, 2015

Frankly, I don't know. It was coercing to bool and I didn't want to change the behavior, this way it still coerces what's left (i.e. not iterable) to bool.
Let me know what you prefer and I'll change it.

This comment has been minimized.

@justanr

justanr Dec 29, 2015

Contributor

It looks like - unless I missed something - that partial is already coerced into a bool with if partial checks and None has no special significance. So a regular tuple should maintain the behavior.

This comment has been minimized.

@tdevelioglu

tdevelioglu Dec 29, 2015

I'm a little bit confused.

If you look closely, it was assigning the coerced value to partial if partial wasn't None, and I kept that behavior (with the obvious exception, as we don't want to convert a tuple value).

Perhaps you could write down what you'd prefer and I'll change it.

This comment has been minimized.

@justanr

justanr Dec 29, 2015

Contributor

That's the only place where partial is referenced in Unmarshaller and it's a simple if partial check. Changing the default of partial to () -- an empty tuple -- allows doing if field_name in partial there instead.

I misunderstood partial is None before, it's a check in the schema to decide which partial value to use. Instead, if we're using a tuple to declare specific fields to partial, why not do partial = partial or self.partial.

This comment has been minimized.

@justanr

justanr Dec 30, 2015

Contributor

Hmm. That's a good point. I think @sloria should make the call if this should be broken.

This comment has been minimized.

@sloria

sloria Jan 11, 2016

Member

I'm not understanding how changing this code to

partial = partial or self.partial

results in a breaking change. I am clearly missing something. The tests pass with this change; can you give an example of code that would break?

This comment has been minimized.

@tdevelioglu

tdevelioglu Jan 11, 2016

With partial = partial or self.partial if you pass False, tuple() or anything else that gets implicitly evaluated false, you'll end up using self.partial. So you need to explicitly check for the default value (None).

For example:

s = Schema(partial=True)
s.load({}, partial=False)  # partial == True

We can drop the branch if it's OK to not coerce things into bool anymore though.

This comment has been minimized.

@sloria

sloria Jan 11, 2016

Member

We can drop the branch if it's OK to not coerce things into bool anymore though.

Yes, that is fine.

This comment has been minimized.

@tdevelioglu
assert 'bar' not in data
assert 'baz' not in data
assert not errors

This comment has been minimized.

@sloria

sloria Dec 29, 2015

Member

Could you also add a test for overriding the partial attribute in load?

e.g.

schema = MySchema(partial=True)
schema.load(.... , partial=('foo', ))

This comment has been minimized.

@tdevelioglu
@sloria

This comment has been minimized.

Member

sloria commented Dec 29, 2015

This looks good. Just a few suggestions above.

Thank you for adding docs and tests.

@tdevelioglu tdevelioglu force-pushed the tdevelioglu:specify_fields_in_partial branch 3 times, most recently Dec 29, 2015

@tdevelioglu tdevelioglu force-pushed the tdevelioglu:specify_fields_in_partial branch Jan 12, 2016

Taylan Develioglu
Support passing specific fields to `partial`
Support passing specific fields to `partial`. This restricts the fields
allowed to be missing during partial deserialization to those specified
in the value of `partial`.
This is particularly useful in api's when dealing with non client-generated
primary key fields (at creation), where only a specific field is allowed
to be missing, and you'd still like to use the same schema.

@tdevelioglu tdevelioglu force-pushed the tdevelioglu:specify_fields_in_partial branch to ecaa077 Jan 13, 2016

@sloria sloria merged commit ecaa077 into marshmallow-code:dev Jan 13, 2016

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
@sloria

This comment has been minimized.

Member

sloria commented Jan 13, 2016

Thanks!

@ewang

This comment has been minimized.

Contributor

ewang commented Apr 7, 2016

@sloria @tdevelioglu I was just going over the changelog to upgrade and noticed this PR. Doesn't passing in MySchema(partial=True, exclude=[...]) achieve the same effect as the merged PR's change of passing in MySchema(partial=(...))

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