On Sat, 2009-01-24 at 14:13 -0800, mrts wrote:
> After several discussions with Honza, we are still on somewhat
> different positions what the validator function signature should
> be and how core validators should access the fields of a form or
> a model instance.
The second bit is relatively minor. We're in complete control of that
and can write the validators however is needed. Getting the former bit
right is more important, since that's the public API.
Leading off with a slight summary of what follows: I suspect this is,
ultimately, a bit of a coin toss. I'm inclined to come down more in line
with Honza's feelings. I've debated a lot with myself whether this is
because a lot of that was originally the same as the ideas I had or
because I prefer it technically, and I'm pretty sure it's the latter.
In either case, there's some roughness in the API simply because models
and forms are not parallel sorts of objects. So some shoe-horning is
required somewhere. How many layers of cotton wool ... er.. abstraction
to wrap around things is the question and it's good that there are
multiple possibilities being discussed, since it means we can have some
confidence the problem has been looked at from multiple angles.
> In core validators, no special case handling of forms and models
> is needed even in multi-value validators. All what is needed is
> an abstraction of "the set of other values".
> AlwaysMatchesOtherField, RequiredIfOtherFieldDoesNotEqual etc
> will work with that abstraction, primary keys and other complex
> model fields also work for same-type comparison.
>
> My initial idea was to reduce the model instance to a dict (as
> discussed previously), however, this is really too restrictive.
> Passing the form or model instance and treating it in dict-like
> manner provides much more flexibility.
Indeed. For models, the whole self.__dict__.copy() stuff was looking a
bit ugly, when "self" is lying around begging to be used. I understand
that it's an attempt to unify data access in reusable validators, but
there has to be a nicer way.
> That can be achieved by the following:
>
> class Model(...):
> ...
> def get_value(self, field_name, *args):
> "Default is specified with the first positional argument"
> if args:
> return getattr(self, field_name, args[0])
> return getattr(self, field_name)
>
> class BaseForm(...)
> ...
> def get_value(self, field_name, *args):
> "Default is specified with the first positional argument"
> if args:
> return self.cleaned_data.get(field_name, args[0])
> return self.cleaned_data[field_name]
>
> (It's not possible to implement the __getitem__ protocol as it
> already works differently in forms.)
>
> In this case the form/model dichotomy is not required in
> validator signature, simple validators work as follows:
>
> def is_slug(value, instance=None):
> (access only value)
>
> and multi-value validators as follows:
>
> class AlwaysMatchesOtherField(object):
> ...
> def __call__(self, value, instance=None):
> if value != instance.get_value(self.other):
> raise ValidationError(...)
>
> There are currently no model-specific complex validators.
In the standard set you have written so far. There will be in what
people write for custom stuff. I'm not quite sure how you define
"complex", though.
> If a
> need for them should arise, it's probably easiest to override
> validate() in the corresponding class, calling super().validate()
> and then perform any additional checks there.
>
> Custom validators can either use the get_value() abstraction to
> support both models and forms or be model/form-specific and use
> the instance in whatever way they like. Checking whether you deal
> with a model or form instance is generally not required -- if you
> accidentally pass a custom form validator to a model, it will
> visibly break and vice-versa.
>
> Honza doesn't like this, mainly because it's too easy to shoot
> yourself into foot with this (as outlined above, I disagree with
> that) and as of now we are using the following instead to handle
> model and form fields uniformly:
>
> def _get_value(name, all_values, instance, do_rise=True):
> if instance is not None and hasattr(instance, name):
> return getattr(instance, name)
> if name in all_values:
> return all_values[name]
> if do_raise:
> raise AttributeError('%s not found in form values or model
> instance'
> % name)
> return False
The idea of a utility function looks appropriate to me. It feels a bit
more explicit and doesn't force policy. I'd like to tweak the
implementation (name, parameters, return types and error messages) but
the idea is correct.
[...]
> Honza's comments:
> -----------------
>
> The problem is that I do not want Form class to be passed to
> validators for several reasons:
Agreed. For models, instance=self, similarly, has to be right, since we
have a more-or-less persistent instance that we are operating on. Forms
are a transient entity (they pop into existence to validate data and
then go away), so "instance" isn't such a useful concept there.
> Form shouldn't be anything except something that obtains data
> from request and can validate them, passing it validators could
> promote adding some methods and properties to be used in
> validators, instead of in the form's clean() method.
>
> Form and model treat data differently, that's just the way it is
> and as it should be - pretending it's not true will just create
> opportunity for nasty surprises.
>
> Validators wouldn't work outside forms and models (it's easy to
> pass in a dictionary of all_values, but fabricating a class with
> the same method is work).
I don't quite understand what this means, but I kind of see what Honza
is getting at. However, I suspect we might need a utility function of
some kind for getting the data value if we want to avoid passing in
self.__dict__ (no copy needed, surely) as the all_values for model
validators. But it can be just a utility function. A class shouldn't be
necessary.
>
> I would still have to check whether I am dealing with model or
> form when I would like to access some model's methods and
> properties that cannot be retrieved this way.
That's kind of a side-issue and not a reason not to pass around the
instance (i.e. not something to drive the API reasoning). But it does
seem to provide a good way to check things at the moment and we could
lock that in.
Again, I'm kind of leaning towards what you have at the moment (Honza's
preferred approach), but it's only a slight lean. Probably not worth
sweating over too much, I suspect, since if somebody really doesn't like
that API, they can use your wrapper class approach in the validators
they write for themselves.
Regards,
Malcolm
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Django developers" group.
To post to this group, send email to django-developers@googlegroups.com
To unsubscribe from this group, send email to
django-developers+unsubscr...@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/django-developers?hl=en
-~----------~----~----~----~------~----~------~--~---
