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

add drop support to Sequence #225

Merged
merged 3 commits into from
Jan 16, 2016

Conversation

tisdall
Copy link
Contributor

@tisdall tisdall commented May 13, 2015

To deal with #224. Sequence/SequenceSchema now supports null and drop in the same way Mapping does.

@tisdall
Copy link
Contributor Author

tisdall commented May 13, 2015

Okay, tests in place.

@tisdall
Copy link
Contributor Author

tisdall commented Jun 18, 2015

I'm thinking this functionality should be moved to SchemaNode.serialize() and SchemaNode.deserialize() instead of being implemented in each separate type. Otherwise, independently created types won't properly handle colander.drop.

@tisdall tisdall changed the title copy null and drop support to Sequence [do not commit] copy null and drop support to Sequence Jun 18, 2015
@tisdall
Copy link
Contributor Author

tisdall commented Jun 18, 2015

I guess handling drop only makes sense in particular types that are collections of other SchemaNode. So, it'd require some sort of check in SchemaNode to determine if we're dealing with a collection type and then iterating over values looking for a colander.drop. Doing that with a cstruct would be easy, but doing it with an appstruct would be very tricky.

I'm leaning towards just implementing this in Sequence and Tuple and then modifying the documentation to indicate that the handling of missing=colander.drop is handled by the container type and any particular container may or may not implement that feature.

What do other people think about this?

@tisdall
Copy link
Contributor Author

tisdall commented Jun 23, 2015

Looking at this some more... drop on a tuple element doesn't make much sense since the number of elements has to remain the same. So, I think this patch is good, the documentation just needs fixing up.

@pallix
Copy link

pallix commented Jun 23, 2015

But should a tuple having (value, None) be dropped of its parent collection or not?

@tisdall
Copy link
Contributor Author

tisdall commented Jun 23, 2015

@pallix - can you elaborate? I don't understand.

My point is as follows:

Let's say you have a TupleSchema like this:

class Coord(colander.TupleSchema):
    x = colander.SchemaNode(colander.Int(), missing=colander.drop)
    y = colander.SchemaNode(colander.Int(), missing=colander.drop)
    z = colander.SchemaNode(colander.Int(), missing=colander.drop)

The result of serialization should always be a 3-element tuple in this case. It doesn't make sense to drop an element as you would with a dict or list because there's no way to figure out which element was the one dropped. For example, if the serialized value was (3, 2), which coordinate axis is missing? Same issue with deserializing a tuple with not enough elements. If you really want to drop values then you should be using a Mapping in this case.

@pallix
Copy link

pallix commented Jun 23, 2015

Ok I see your point ; I'm relatively new using this library so I will trust your competence.

@tisdall
Copy link
Contributor Author

tisdall commented Jun 23, 2015

@pallix - new... old... if you see something wrong with my logic, let me know. :)

@tisdall
Copy link
Contributor Author

tisdall commented Jun 23, 2015

I know right now there's no "validation" on missing values, but I'm wondering if an exception should be thrown when you try to using missing=colander.drop in situations it doesn't make sense (such as on a TupleSchema). Thoughts?

@pallix
Copy link

pallix commented Jun 23, 2015

It's always nice to have as much validation as possible as earlier as possible.

@tisdall tisdall changed the title [do not commit] copy null and drop support to Sequence add drop support to Sequence Jul 2, 2015
@tisdall
Copy link
Contributor Author

tisdall commented Jul 2, 2015

Okay, I think this is good to be merged now.

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