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

suggestion: only='string' #316

Closed
paulocheque opened this Issue Oct 29, 2015 · 11 comments

Comments

Projects
None yet
7 participants
@paulocheque

paulocheque commented Oct 29, 2015

It took me time to understand what is happening when I passed a ('string') instead of ('string',). Maybe it would be a good idea to raise an error or accept one unique field in the only field.

@sloria

This comment has been minimized.

Member

sloria commented Nov 11, 2015

Fair point, though I'm not sure which alternative is better:

  1. Raise an error if only isn't a collection
  2. Allow only to be a string. What would the behavior be here? Would only='field' be equivalent to only=('field', ) or would it have different behavior, like passing only='field' to Nested?

I am open to suggestions.

@paulocheque

This comment has been minimized.

paulocheque commented Nov 11, 2015

I would choose the option 2 because it gives flexibility to the final code. But I think it is better to mantain the simplicity of the tool, avoiding too much useless flexibility. So I would go with the option 1.

@density

This comment has been minimized.

density commented Dec 5, 2015

It's probably "pythonic" to be forgiving but I think it's better to force users to be explicit. And why have two ways to express the same thing? Option 1 👍

@xuhcc

This comment has been minimized.

xuhcc commented Dec 16, 2015

Just ran into the same problem with exclude='field'. I think an exception should be raised.

@sloria sloria added this to the 3.0 milestone Dec 28, 2015

@frol

This comment has been minimized.

Contributor

frol commented Mar 6, 2016

+1 for Option 1.

@deckar01

This comment has been minimized.

Member

deckar01 commented Jun 27, 2016

I am leaning towards option 2.2 (like passing only='field' to Nested) which plucks the attribute when only is a string.

My reasoning is that it would simplify the implementation by moving the custom only handling from the Nested field to the Schema class, and would normalize the behavior of the only parameter across interfaces.

But I think it is better to mantain the simplicity of the tool, avoiding too much useless flexibility.

This functionality is already present in the Nested field and it is useful. The simplest thing to do would be to push the logic down to the schema implementation.

Consider an API that needs to expose a flat list of user names. Currently you would have to pluck the fields out of the list of objects manually. I think this is a fundamental serialization operation that belongs in the schema implementation.

user_names_schema = UserSchema(only='name')
data, errors = user_names_schema.dumps(users, many=True)
# '["username1", "username2", "username3"]'

This is also related to loads() handling this behavior for the inverse operation (see #314).

user_names_schema.loads(data, many=True)
# [{"name": "username1"}, {"name": "username2"}, {"name": "username3"}]
@sloria

This comment has been minimized.

Member

sloria commented May 14, 2018

This is closely related to #800, where it's being proposed that we remove the "plucking" behavior when passing a string to only.

If we move forward with that, then it probably makes sense to go with option 1 and disallow non-collection input to only.

@timc13

This comment has been minimized.

timc13 commented Jul 25, 2018

i think it's better to be forgiving and go with option 2, rather than a runtime error

@deckar01

This comment has been minimized.

Member

deckar01 commented Jul 25, 2018

I think modulating the Nested functionality this much based on the type of a parameter is confusing.

#800 (comment)

@timc13 The same argument applies here. I am in the process of removing the string handling for only in Nested in favor of a dedicated Pluck field. I'm not sure adding string handling for only is the best way to support this feature during schema instantiation. It would be nice if fields could be loaded and dumped without a schema to wrap them.

user_names_schema = fields.Pluck(UserSchema, 'name', many=True)
data, errors = user_names_schema.dumps(users)

@sloria I have been experimenting with treating schema as an dict/object field internally. This allows all fields (including non-collection types) to be used like a schema. It also eliminates the need for the many schema constructor argument in favor of wrapping the schema in a list field.

https://deckar01.gitlab.io/marsha/guide.html#deserializing-data

@timc13

This comment has been minimized.

timc13 commented Jul 25, 2018

I suppose the argument is that this is not modulating the behavior of only, but only allowing forgiveness because it is easy to pass in a string out of convenience when you only want one field.

This would just always transform a single string into a collection of strings.

By the way, this logic should apply to both only and exclude arguments

@sloria

This comment has been minimized.

Member

sloria commented Aug 4, 2018

Went with option 1 (raise an error) in #896. Together with the upcoming Pluck field, I think it's a good approach.

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