Fixes issue #87. #88

Closed
wants to merge 1 commit into
from

Projects

None yet

3 participants

@AndreLouisCaron

Adds deferred preparer implementation :-)

@jayd3e
Pylons Project member

I'll make sure this gets merged either today or tomorrow.

@mcdonc
Pylons Project member

I'm not sure I understand this patch. It seems its effect is to avoid calling the preparer if deserialize is called on an unbound schema. However, if you do:

newschema = schema.bind(...)
newschema.deserialize(....)

There's no need to avoid trying to call the preparer if it's deferred, because after a bind nothing will be deferred. And it's already possible to defer a preparer as long as you call bind before deserialize.

Am I missing something?

@AndreLouisCaron

@mcdonc This patch allows preparers to use context injected by binding the schema, which is otherwise not accessible to preparers.

Even though all preparers are naturally "deferred" (they are only called during deserialization), the implementation must avoid calling the preparer when the schema is not bound because .deserialize() doesn't know what to use as the kw argument.

The purpose of the patch is not to avoid calling the preparer, but rather this must be done in order for the patch to work.

@jayd3e
Pylons Project member

So basically, you want to be able to use the kwargs that you pass in through bind, in your preparers?

@mcdonc
Pylons Project member

Given that you can already use a bound schema with a deferred preparer, is there a specific case where you'll personally want to use an unbound schema with a deferred preparer? I only ask because there's another effort afoot in the issue tracker to fail hard when people try to use deferred along with unbound schemas that I think I might agree with.

@AndreLouisCaron

@mcdo No, I don't have a use case for using deferred with unbound schemas, I only supported that use case because it seems to be supported by other uses of deferreds.

As for the more general "using unbound schemas" issue, I think the entire schema binding is much too complex for its own good. It would be much simpler if the context was passed as an argument to .deserialize() and if it was forwarded to all "validators", "missings" and "preparers". This would avoid the bound/unbound duality entirely. Then, when people use validators/missings/preparers that require context which was not provided, the validator/missing/preparer can just raise a MissingContext exception or something.

@mcdonc
Pylons Project member

OK, understood. I'm going to reject the patch given that you don't specifically need the feature, as it's likely we're going to remove support for using deferreds along with an unbound schema at some point.

On your other point, it's now possible to subclass SchemaNode and use the "bindings" attribute inside methods and properties of the subclass instead of using deferreds. If you search for "bindings" in http://docs.pylonsproject.org/projects/colander/en/latest/basics.html#inheriting-schemas you can see an example.

@mcdonc mcdonc closed this Mar 19, 2013
@AndreLouisCaron

@mcdonc I'm not sure I understand the basis for rejection. If you prefer that I transform the implementation into this:

if isinstance(preparer, deferred):
    raise UnboundSchemaError(...)
appstruct = preparer(appstruct)

then that's fine with me. I'm ready to do the changes to get the right behaviour right now, why do you want to postpone this to a later (and unspecified) time?

The example you're referring to is for validators. AFAIK, nothing in the docs seem to support this syle for preparers. If there is similar support for preparers and you can add it to the documentation instead of this patch, then that would be fine with me.

@jayd3e
Pylons Project member

The API is changing for validators, and as such it is going to effect preparers as well. Basically what is going to happen is the All() validator will be deprecated, and the new api will be passing iterables to validator/preparer. Some code will then be written to support both deferreds in both simultaneously.

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