Honza Král wrote:
> I will be glad on any feedback. As soon as I figure these things out,
> I can finish the work and try and push for inclusion.
My notes, comments, and suggestions with the patch are below. Hopefully, this
will prove useful to those sprinting on this tomorrow.
validators
==========
* New validators (replacements for many, but not all of the old validators)
have been put in ``django.core.validators``, and the old validators that used
to live there have been moved to ``django.oldforms.validators``.
* Not all of the old validators have replacements. We are probably going to
want to bring over any that haven't been converted already.
* The new validators are only passed the value for the single field, whereas
the old validators received both the value for the single field and the values
for all fields.
* The new validators all have message_dict parameters, which I assume is
supposed to be similar to the error_messages of of form Fields. If this is
supposed to mimic error_messages of form Fields, then I think we should rename
it to match. It doesn't look it's being used yet, and this is going to
require either:
1. Moving all the error message strings to a local dict within the
validator function. Any error messages passed to the validator will override
the defaults.
2. Making a Validator class (that all validators inherit from) to handle
the error_message overriding. This would also be more future proof since
validators become objects instead of just functions.
* The ValidationError class has moved from forms.utils to
django.core.exceptions now that it is used for more than just forms.
contrib.admin.views.template
============================
* I think the import here should be "from django.core import exceptions"
instead of "from django.core import validation". The raise statement needs
correcting too.
django.db.models.Model class
============================
* The methods to_python() and clean() have been added, and the old validate()
method has been changed slightly.
* clean() simply calls to_python(), then validate().
- to_python() calls each field's to_python() and builds up an error_dict of
errors. If there are errors, a ValidationError is raised.
- validate() runs the field values through any validate_<field> methods
defined on the Model, building up an error_dict of errors. It will raise a
ValidationError if there are any errors.
- One thing I notice here is that if to_python() has any errors, none of
validate() will run. This means that you could have a field that shows no
errors one time, and then starts showing errors the next time if there was at
least one error in to_python() the first run and then no errors in to_python()
but errors in validate() the next run. The old validate() method seemed to
handle this better since it attempted to do both field.to_python() and
field.validate_full() in one fell swoop, skipping any field.validate_full()'s
for fields that didn't make it through the field.to_python().
Model fields (django.db.models.fields)
======================================
* Model fields get a new "validators" attribute that is a list of "new"
validators. The "validator_list" __init__ parameter also still exists for
oldforms compatibility and we can remove it when we remove oldforms.
* The validate_full() method has been renamed to validate().
- There is a comment here suggesting we add the unique and unique_for_* to
the Model's validate_<field> method(s) via contribute_to_class, but I think
it's perfectly fine where it is. These are options available to every field,
so it makes sense to perform these checks in the Field class. Dynamically
adding the checks to various validate_<field> methods seems messy and complex
(how would you then override a validate_<field> method, for example).
* There are fields now using the "old" validators that have been moved to
django.oldforms.validators. It looks like all of these are in
oldforms-related methods, such as get_manipulator_fields(), so this is OK.
They will be removed with oldforms. However, some of these being used are of
the RequiredIfOtherFieldNotGiven variety. Is this same functionality
available to the mechanisms used by newforms?
Form Fields (django.forms.fields)
=================================
* Form Fields get a new "validators" attribute that is a list of "new" validators.
* The to_python() method remains and a validate() method have been added.
Fields have had their clean() methods split into to_python() and validate()
methods.
* clean() now calls to_python() then validate().
django.forms.forms.BaseForm class
=================================
* Form gets a new validate() method, which is meant to replace clean(), and
full_clean() now looks for validate_<field> methods, which are meant to
replace clean_<field> methods. full_clean() also uses the Form.validate()
instead of Form.clean() to set the cleaned_data.
- The validate_<field> methods are passed the Field.clean()'ed value (yea
for fixing #3896).
- The clean() method and calling of clean_<field> methods wasn't removed,
for backwards compatibility. If these are going to be deprecated (and I think
they should), and if we are to follow our official release policy of:
"Major point releases will always remove deprecated features immediately."
...then we should just go ahead and remove them now.
django.forms.models.BaseModelForm class
=======================================
* Gets a validate() method that calls BaseForm.validate(), then
self.instance.clean().
- If there are any errors after the BaseForm.validate(), then
self.instance.clean() is not called. I think we want to be calling both here
and collecting all the errors, otherwise we have the same problem mentioned
earlier where two levels of errors are possible and you don't reach the second
level of errors until there are no errors in the first level.
django.forms.models.BaseInlineFormset class
===========================================
* Gained a _construct_form() method. I'm not familiar at all with the Formset
functionality, so I'm not exactly sure why this method was added (a docstring
would be nice).
Suggestions
===========
clean() vs. validate()
~~~~~~~~~~~~~~~~~~~~~~
One thing that annoys me a bit is the fact that, for Models and Forms, clean()
is the higher level method that calls to_python() and validate(). While I
think the bikeshed would look nicer with validate() being the high level
method, calling to_python() and clean(), where you would have something like:
user = User(first_name='jane', last_name='doe')
user.validate()
...instead of:
user = User(first_name='jane', last_name='doe')
user.clean()
...I guess I wouldn't really like how that would mean that instead of
validators we would have cleaners, bah.
Model.is_valid()?
~~~~~~~~~~~~~~~~~
An is_valid() method on Model could be convenient, as it is on Forms. This
way, you could just do a check with is_valid():
user = User(first_name='jane', last_name='doe')
if user.is_valid():
user.save()
else:
...
...instead of calling clean() and having to catch the ValidationError:
user = User(first_name='jane', last_name='doe')
try:
user.clean()
except ValidationError:
...
user.save()
Should Model.save() validate first?
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Since the things that model validation is doing are defined as options on the
Model and its fields, it seems that a save() should validate first. This
could be combined with the is_valid() above along with caching the errors on
the Model instance so that subsequent calls to is_valid() are no-ops. That
way you could explicitly call is_valid() before calling save, and when you do
call save, validation wouldn't need to be rerun.
Gary