Code review comment for lp:~rvb/maas/multifield

 status approve

On Tuesday 05 Jun 2012 09:06:22 Raphaël Badin wrote:
> > I think SKIP_CHECK_NAME needs to be more obscure; it's quite conceivable
> > that someone will have a field named skip_check. Perhaps '__skip_check'
> > ?
> You've got a point, but since SKIP_CHECK_NAME is exposed to the user, I
> think it should be readable. Instead I've added a check to make sure that
> no user-defined subfield is named 'skip_check'. I've added a test for that
> (test_DictCharField_does_not_allow_subfield_named_skip_check).

This is good that you've checked its presence - however I'd make the error
message convey more information like:

"skip_check is a reserved name"

so the user knows why it's disallowed.

+ """Extract the values for each widget from a data dict (QueryDict).
+ :param data: The data dict (usually request.data or request.GET where
+ request is a django.http.HttpRequest).
+ :param data: dict
+ :param files: The files dict (usually request.FILES where request is
a
+ django.http.HttpRequest).
+ :param files: dict
+ :param name: The name of the widget.
+ :param name: basestring

Some of these s/param/type/

Everything else is much better! I particularly like the doctest-like nature
of some of the docstrings, thank you.

Approved with the change above!