newforms generic views
Gary Wilson Jr.
Brian Rosner for the hard work), and trying to decide how backwards
compatible we want to be. (I should also mention that while there has
been some work done towards class-based generic views in #6735 [3], I
believe that #3639 should be completed first as #6735 could be done
post-1.0 if need be.) So, I come to the community for input...
Currently the create_update views take a required ``model`` argument
and an optional ``follow`` argument. The ``model`` argument is fine and
we can carry that forward. The ``follow`` argument, however, is
specific to oldforms Manipulators and was used for showing/hiding form
fields (see [2] for a refresher of the follow argument).
In order to enable custom newforms-style forms, Brian has added a
``form_class`` argument to the views, which I think is the correct way
to replace the functionality lost by the ``follow`` argument.
There are a couple design decisions that need to be made, though:
1. Brian's patch replaces the required ``model`` argument with the
required ``form_class`` argument, where ``form_class`` can either be a
``forms.ModelForm`` subclass or ``model.Model`` subclass.
a. I am thinking that we should instead keep the ``model`` argument,
but make it optional. Then, we ensure that one of ``model`` or
``form_class`` is given. ``form_class``, if given, would override
``model`` or if just ``model`` was given, then a ModelForm would be
created for the passed model. Does this sound reasonable?
b. Anyone have any other ideas here?
2. What should we do with the ``follow`` argument?
a. We could drop it completely, which would not be backwards
compatible for anyone using the ``follow`` argument.
b. We could issue a deprecation warning if ``follow`` is used,
letting people know that generic views now use newforms and to use
``form_class`` if you need a custom form. This would be a bit more
backwards compatible, since if you aren't using ``follow`` everything
should work the same. If you are using ``follow``, then those forms
might display/behave differently (i.e. fields you were trying to hide
now get displayed).
c. We could be even more backwards compatible by trying to take
fields declared in ``follow`` and make them includes/excludes in the
inner Meta class of the generated ModelForm.
I have taken Brian's latest patch and added implementations of 1a and
2b. Other additions were:
* Fixed an error I was getting in the tests when using "model = model"
in the inner Meta class (works fine in my shell, but gives me model not
defined errors when I run the tests) by introducing a tmp_model variable.
* Added a GenericViewError class and made a couple AttributeErrors use
this Exception class instead since AttributeError didn't really fit.
* Added a get_model_and_form_class helper function to remove duplicate
ModelForm-generating code.
* Finished off the test_create_custom_save_article test with a
custom_create view that passes a custom form to the create_update
generic view.
I have attached my patch [4] to the ticket.
Gary
[1]
http://code.djangoproject.com/attachment/ticket/3639/create_update_newforms5.diff
[2] http://code.djangoproject.com/wiki/NewAdminChanges
[3] http://code.djangoproject.com/ticket/6735
[4] http://code.djangoproject.com/attachment/ticket/3639/3639.diff
Brian Rosner
On Jun 16, 2008, at 2:51 PM, Gary Wilson Jr. wrote:
>
> I was taking a look at the latest patch [1] for #3639 (many thanks to
> Brian Rosner for the hard work), and trying to decide how backwards
> compatible we want to be. (I should also mention that while there has
> been some work done towards class-based generic views in #6735 [3], I
> believe that #3639 should be completed first as #6735 could be done
> post-1.0 if need be.) So, I come to the community for input...
I personally agree that #3639 should be what is used in 1.0 and then
post-1.0 can introduce the class-based view enhancements.
> In order to enable custom newforms-style forms, Brian has added a
> ``form_class`` argument to the views, which I think is the correct way
> to replace the functionality lost by the ``follow`` argument.
>
> There are a couple design decisions that need to be made, though:
>
> 1. Brian's patch replaces the required ``model`` argument with the
> required ``form_class`` argument, where ``form_class`` can either be a
> ``forms.ModelForm`` subclass or ``model.Model`` subclass.
I was going to rename ``form_class`` to ``model`` but never got around
to it. It was to be more backward compatible, which still sounds
reasonable to me.
> a. I am thinking that we should instead keep the ``model`` argument,
> but make it optional. Then, we ensure that one of ``model`` or
> ``form_class`` is given. ``form_class``, if given, would override
> ``model`` or if just ``model`` was given, then a ModelForm would be
> created for the passed model. Does this sound reasonable?
newforms-admin will bring in a modelform_factory that could be used.
But in the meantime this would be fine with me.
> 2. What should we do with the ``follow`` argument?
>
> a. We could drop it completely, which would not be backwards
> compatible for anyone using the ``follow`` argument.
I wouldn't be opposed to this.
>
>
> b. We could issue a deprecation warning if ``follow`` is used,
> letting people know that generic views now use newforms and to use
> ``form_class`` if you need a custom form. This would be a bit more
> backwards compatible, since if you aren't using ``follow`` everything
> should work the same. If you are using ``follow``, then those forms
> might display/behave differently (i.e. fields you were trying to hide
> now get displayed).
+1 here. I don't know much about ``follow``, but I wouldn't want to
see some legacy support for it while we get to 1.0. It should break
cleanly now and then horribly later IMHO.
>
>
> c. We could be even more backwards compatible by trying to take
> fields declared in ``follow`` and make them includes/excludes in the
> inner Meta class of the generated ModelForm.
Yuck :)
Brian Rosner
http://oebfare.com
Jacob Kaplan-Moss
> a. I am thinking that we should instead keep the ``model`` argument,
> but make it optional. Then, we ensure that one of ``model`` or
> ``form_class`` is given. ``form_class``, if given, would override
> ``model`` or if just ``model`` was given, then a ModelForm would be
> created for the passed model. Does this sound reasonable?
Yes, very much so. I'd like to call it ``model`` and ``form`` (instead
of ``form_class``, which is redundant), but Brian's building the shed,
so he can paint it any color he likes.
[The cute thing about allowing any form, by the way, is that if you
give it something that *looks* like a ModelForm -- i.e. defines save()
-- you can actually use the view with any form you like... Me likey.]
> 2. What should we do with the ``follow`` argument?
[...]
> b. We could issue a deprecation warning if ``follow`` is used,
> letting people know that generic views now use newforms and to use
> ``form_class`` if you need a custom form. This would be a bit more
> backwards compatible, since if you aren't using ``follow`` everything
> should work the same. If you are using ``follow``, then those forms
> might display/behave differently (i.e. fields you were trying to hide
> now get displayed).
+1 here. I'd say issue DeprecationWarning until 1.0 beta, then drop it entirely.
I'll have a look at the patch itself, but from your description it
sounds like this is looking good.
Jacob
Gary Wilson Jr.
> On Mon, Jun 16, 2008 at 3:51 PM, Gary Wilson Jr. <gary....@gmail.com> wrote:
>> a. I am thinking that we should instead keep the ``model`` argument,
>> but make it optional. Then, we ensure that one of ``model`` or
>> ``form_class`` is given. ``form_class``, if given, would override
>> ``model`` or if just ``model`` was given, then a ModelForm would be
>> created for the passed model. Does this sound reasonable?
>
> Yes, very much so. I'd like to call it ``model`` and ``form`` (instead
> of ``form_class``, which is redundant), but Brian's building the shed,
> so he can paint it any color he likes.
I assume that ``form_class`` was used because it actually needs to be a
form class and not a form instance (the views take the form class and
instantiate a form with the post data). So I would say it should
probably stay ``form_class``.
>> 2. What should we do with the ``follow`` argument?
> [...]
>> b. We could issue a deprecation warning if ``follow`` is used,
>> letting people know that generic views now use newforms and to use
>> ``form_class`` if you need a custom form. This would be a bit more
>> backwards compatible, since if you aren't using ``follow`` everything
>> should work the same. If you are using ``follow``, then those forms
>> might display/behave differently (i.e. fields you were trying to hide
>> now get displayed).
>
> +1 here. I'd say issue DeprecationWarning until 1.0 beta, then drop it entirely.
Cool, this is what the current patch does.
> I'll have a look at the patch itself, but from your description it
> sounds like this is looking good.
>
> Jacob
Gary