ModelForm fieldsets?
Tom Tobin
newforms-admin) shouldn't be pushed down into the newforms library?
What I'm doing right now by overriding change_form.html would be even
easier if I could define my fieldsets on a ModelForm.
Related to how I'm working around it — it would be nice if the
change_form template had a {% block field_sets %} around the fieldset
include section. (I suppose I'll punch up a ticket and a patch.)
Malcolm Tredinnick
On Fri, 2008-02-08 at 16:08 -0600, Tom Tobin wrote:
> Is there any good reason why the "fieldsets" option of ModelAdmin (in
> newforms-admin) shouldn't be pushed down into the newforms library?
> What I'm doing right now by overriding change_form.html would be even
> easier if I could define my fieldsets on a ModelForm.
A lot of that code (fieldsets and formsets) can eventually come into
trunk. Mostly it's a matter of Joseph being happy with the API and it
being worth any disruption to do it before waiting for newforms-admin
merge (I suspect both features have zero impact when you don't use
them).
Joseph and I have certainly talked about merging formsets before,
although that drags in a bunch of other baggage (Russell's media
handling in forms), so we haven't gotten around to it yet. (I've also
developed concerns over the complexity of formsets lately, which may
have slowed things).
However, restricting fieldsets to ModelForms would be narrow-minded.
Come up with a form-aware way of specifying them that ModelForms can
then use when creating its fieldsets.
Regards,
Malcolm
--
I don't have a solution, but I admire your problem.
http://www.pointy-stick.com/blog/
Brian Rosner
> newforms-admin) shouldn't be pushed down into the newforms library?
> What I'm doing right now by overriding change_form.html would be even
> easier if I could define my fieldsets on a ModelForm.
I don't see reason why it shouldn't be at the newforms level. I agree
with Malcolm and say that this shouldn't be scoped to just ModelForms.
Now that I think about this a little more it would really fix up the
fiddly bits with the ModelAdmin class and the form_add/form_change
methods. It might be a bit too much, but why should there even be
fieldsets on ModelAdmin when this can all be done with a custom form.
This puts me at a +1 on the idea.
--
Brian Rosner
http://oebfare.com
Tom Tobin
>
> > Is there any good reason why the "fieldsets" option of ModelAdmin (in
> > newforms-admin) shouldn't be pushed down into the newforms library?
> > What I'm doing right now by overriding change_form.html would be even
> > easier if I could define my fieldsets on a ModelForm.
>
> I don't see reason why it shouldn't be at the newforms level. I agree
> with Malcolm and say that this shouldn't be scoped to just ModelForms.
Yeah, I never meant to imply that it should be limited to ModelForms;
that just happened to be my use case. ^_^
pm13
someone will do it. So I would like to say that I have patches for
these problems (fieldsets, inlines and more) - through inner Meta
class. They are almost untested (no tests, no ported code, only one or
two forms with them). But I hope I change it and create tickets for
them in week or two.
So if someone is interested in coding for this - please wait or tell
it. It is quite complex and it would be useless to do everything
twice.
Malcolm Tredinnick
On Mon, 2008-02-18 at 11:45 -0800, pm13 wrote:
[...]
> It seems that this idea could be accepted. And it is possible that
> someone will do it. So I would like to say that I have patches for
> these problems (fieldsets, inlines and more) - through inner Meta
> class. They are almost untested (no tests, no ported code, only one or
> two forms with them). But I hope I change it and create tickets for
> them in week or two.
Perhaps before launching out too far with large changes, it would be
worth discussing the API on this list first. It's not completely obvious
how this would all work. Particularly, the "inlines and more" bit makes
me think there's some design work to be discussed first.
Thanks,
Malcolm
--
How many of you believe in telekinesis? Raise my hand...
http://www.pointy-stick.com/blog/
pm13
> On Mon, 2008-02-18 at 11:45 -0800, pm13 wrote:
>
> [...]
>
> > It seems that this idea could be accepted. And it is possible that
> > someone will do it. So I would like to say that I have patches for
> > these problems (fieldsets, inlines and more) - through inner Meta
> > class. They are almost untested (no tests, no ported code, only one or
> > two forms with them). But I hope I change it and create tickets for
> > them in week or two.
>
> Perhaps before launching out too far with large changes, it would be
> worth discussing the API on this list first. It's not completely obvious
> how this would all work. Particularly, the "inlines and more" bit makes
> me think there's some design work to be discussed first.
>
> Thanks,
> Malcolm
created some tickets with the description of the new API and it would
be great to get some feedback (especially for the first and for the
last one).
Fieldsets for newforms:
http://code.djangoproject.com/ticket/6630
Additional options for newforms:
http://code.djangoproject.com/ticket/6631
FormSet, ModelFormSet, InlineFormSet and inlines for newforms in
trunk:
http://code.djangoproject.com/ticket/6632
Special newforms which could be used in newforms-admin (no patch, only
an idea):
http://code.djangoproject.com/ticket/6633
Default widgets and formfields in dbfields (no patch, I only ask for
design decision):
http://code.djangoproject.com/ticket/6634
Yes, I agree that if they are accepted, they have to be change quite a
lot. I don't believe the API is good enough.
Malcolm Tredinnick
On Wed, 2008-02-20 at 12:53 -0800, pm13 wrote:
[...]
> OK - API is one thing and implementation is another things. I have
> created some tickets with the description of the new API and it would
> be great to get some feedback (especially for the first and for the
> last one).
There really is a good reason we ask for the API discussions to happen
on the mailing list: Trac tickets are a terrible place for design
discussions and don't get seen by the right audience. That is explained
in our contributing document. Particularly when a ticket (like #6331)
proposes half a dozen new features, which will mean some will need
tweaking and some won't.
I'm mostly just commenting on the API here. I've only read the patch a
couple of times and haven't fully absorbed the inner details yet.
You've certainly put a lot of work into this and I really don't want to
seem to be arbitrarily dismissing your ideas (which I'm definitely not).
However, I think you've taken a slightly restrictive view of the
use-cases in a number of places as well as being a bit too liberal with
waving the backwards-incompatibility stick around.
> Fieldsets for newforms:
> http://code.djangoproject.com/ticket/6630
Having just spent quite a while making ModelForms (and current model
inheritance) behave consistently with Python's subclassing behaviour,
some of your proposed changes raise red flags for me (and I realise
you've noted this).
The implicit inheritance behaviour you've given to the inner Meta
classes seems wrong. Python asks you to declare base classes explicitly,
with good reason; I'd prefer to stick to that behaviour. It provides a
much clearer way of avoiding accidental inheritance of behaviour: you
simply don't inherit. In your version, you get these parent class
features whether you want them or not and it's a bit fiddly to disable
features. Behaving like Python is a good thing (as are Zen of Python
points #2 and #3).
The new methods on the widgets (for_p, for_ul, for_table) feel awkward,
since as_p, as_ul and as_table on the Form class are just three examples
of the many styles of layout that are possible. One of the nice things
about the current design is that you need to subclass Form anyway and
that's where you put your layout method. Now, you also need to add a new
method to every single widget you use. That's a lot of extra work. The
idea is to avoid hard-coding too much in the way of presentation methods
into Forms so that people can customise easily as they see fit. This way
we don't have to include any more possibilities in Django core. Your
patch actually pushes more presentation information down deeper into the
newforms stack, making overriding harder.
What would my code look like if I decided to turn an as_compact_layout()
method on my Form now into something in your schemes?
(as_compact_layout() is kind of like what you might imagine as_div(),
but the point is, it's an arbitrary presentation method that passes
parameters to Form._as_html()). Say I have a very standard Form with a
few fields declared plus this as_compact_layout() method. Am I going to
have to add a new method to every widget of every field?
Why isn't exclude respected any longer for ModelForms? That's a nice
shortcut. Is there a technical reason it can't be supported?
Renaming formfield_callback to formfield_for_dbfield doesn't seem
worthwhile. Why not leave it with it's current name and avoid
unnecessary breakage to existing code?
If you're going to add new positional arguments add them to the end to
avoid breaking existing code (e.g. row_attrs). That's, again, just
unnecessary breakage when it can be so easily avoided.
BaseForm exists for good reason: it provides an easy way to create
Form-like classes that get their fields in other ways from the current
approach. For most people, that's unimportant and they can just use
Form. I've found it reasonably useful to be able to write a different
__new__ method (replace DeclarativeFieldsMetaclass) and subclass
BaseForm, myself. Changing that now is just re-arranging deck chairs and
doesn't seem to introduce any gain. Your argument in the ticket only
addresses the fact that we have ModelForms. There are other
possibilities in use.
The idea of specifying fieldsets as a list of dictionaries makes sense.
Analogous to how we do field groupings in the existing admin and that
seems to have worked fairly well.
I think a lot of your other presentation-controlling changes reduce
flexibility, though. I like the current Form class design a lot. It
really does strikea nice balance between default layout and allowing
lots of stuff to be controlled without having to write lots of code.
Your changes make this a bit harder (e.g. moving presentation stuff
further down into the widgets). Is all that really necessary just to
make fieldsets work? I'd rather see that separated out, since fieldsets
stirkes me as something that is just grouping existing fields, so
shouldn't require a lot of changes to how they're displayed (you might
well want to make such changes, but are they functionally stopping just
the fieldset changes? If not, they're a separate issue).
>
> Additional options for newforms:
> http://code.djangoproject.com/ticket/6631
There are a lot of separate things there. Many of them seem to be
backwards incompatible without introducing new functionality and without
necessarily making the code much simpler.
If you want to write a custom form, you subclass the Form class, so you
already have the ability to write your own rendering function and
control the error class and the label and things like that. The current
defaults work pretty well, too, for simple stuff.
This mail's already quite long. I'll address each of the alternatives
(plus all the other tickets, in turn) in another mail if nobody else
expresses my opinions first.
[...]
> Default widgets and formfields in dbfields (no patch, I only ask for
> design decision):
> http://code.djangoproject.com/ticket/6634
This one should be brought up on this mailing list. It's not appropriate
for a ticket.
Regards,
Malcolm
--
If Barbie is so popular, why do you have to buy her friends?
http://www.pointy-stick.com/blog/
pm13
> couple of times and haven't fully absorbed the inner details yet.
>
> You've certainly put a lot of work into this and I really don't want to
> seem to be arbitrarily dismissing your ideas (which I'm definitely not).
> However, I think you've taken a slightly restrictive view of the
> use-cases in a number of places as well as being a bit too liberal with
> waving the backwards-incompatibility stick around.
>
> > Fieldsets for newforms:
> >http://code.djangoproject.com/ticket/6630
>
> Having just spent quite a while making ModelForms (and current model
> inheritance) behave consistently with Python's subclassing behaviour,
> some of your proposed changes raise red flags for me (and I realise
> you've noted this).
>
> The implicit inheritance behaviour you've given to the inner Meta
> classes seems wrong. Python asks you to declare base classes explicitly,
> with good reason; I'd prefer to stick to that behaviour. It provides a
> much clearer way of avoiding accidental inheritance of behaviour: you
> simply don't inherit. In your version, you get these parent class
> features whether you want them or not and it's a bit fiddly to disable
> features. Behaving like Python is a good thing (as are Zen of Python
> points #2 and #3).
not sure if my idea was bad or good - I saw reasons for both
possibilities. So I decide to do nothing and to ask.
My use case could be some kind of form with many subclasses and with
different default options. But there is other way - to define custom
option class and set it as a attribute of the form. So after reading
of your comment I think also that I should change it.
> The new methods on the widgets (for_p, for_ul, for_table) feel awkward,
> since as_p, as_ul and as_table on the Form class are just three examples
> of the many styles of layout that are possible. One of the nice things
> about the current design is that you need to subclass Form anyway and
> that's where you put your layout method. Now, you also need to add a new
> method to every single widget you use. That's a lot of extra work. The
> idea is to avoid hard-coding too much in the way of presentation methods
> into Forms so that people can customise easily as they see fit. This way
> we don't have to include any more possibilities in Django core. Your
> patch actually pushes more presentation information down deeper into the
> newforms stack, making overriding harder.
>
> What would my code look like if I decided to turn an as_compact_layout()
> method on my Form now into something in your schemes?
> (as_compact_layout() is kind of like what you might imagine as_div(),
> but the point is, it's an arbitrary presentation method that passes
> parameters to Form._as_html()). Say I have a very standard Form with a
> few fields declared plus this as_compact_layout() method. Am I going to
> have to add a new method to every widget of every field?
ticket #3111 which was accepted by jacob. So I thought it was no
problem to do it.
My use case is: there is a big form with 15 fields and 3 fieldsets. I
use in template only "{{ form.as_ul }}". But someone tells me that one
widget should be a little different. What shall I do now? With my
patch I only create custom widget and use it.
I think your use case can be solved for example by adding new method
of Form which would only call method of the widget. And you would
overwrite only this method - all widgets would be unchanged.
> Why isn't exclude respected any longer for ModelForms? That's a nice
> shortcut. Is there a technical reason it can't be supported?
fields option. My reason is that there would be three possible methods
for definition of fields (fieldsets, fields, exclude) and the rule
"use only one of them" seems to be simple and powerful enough. Are
there any situations where fields and exclude are needed both?
> Renaming formfield_callback to formfield_for_dbfield doesn't seem
> worthwhile. Why not leave it with it's current name and avoid
> unnecessary breakage to existing code?
DeclarativeFieldsMetaclass.__new__. So it is inaccessible for the
common form construction. My primary reason was to move it to the
inner class Meta. It is a part of the bigger idea to have inner class
"Meta" which would be used for common customization. I would not
change the name without the move.
The second reason is the metaclasses and their methods are more pretty
if they have same arguments. The argument formfield_callback seems to
be very inconsistent. But it not really necessary.
> If you're going to add new positional arguments add them to the end to
> avoid breaking existing code (e.g. row_attrs). That's, again, just
> unnecessary breakage when it can be so easily avoided.
but not from any core developer.
I start with the previous use case:
There is a big form with 15 fields and 3 fieldsets. I use in template
only "{{ form.as_ul }}". But someone tells me that he needs special
style for one row. What shall I do now? With my patch I only add
row_attrs to widget constructor.
It is more simple variant of output methods in widget - I think that
in the most situations I only need set id or class for row and use css
for customization.
> BaseForm exists for good reason: it provides an easy way to create
> Form-like classes that get their fields in other ways from the current
> approach. For most people, that's unimportant and they can just use
> Form. I've found it reasonably useful to be able to write a different
> __new__ method (replace DeclarativeFieldsMetaclass) and subclass
> BaseForm, myself. Changing that now is just re-arranging deck chairs and
> doesn't seem to introduce any gain. Your argument in the ticket only
> addresses the fact that we have ModelForms. There are other
> possibilities in use.
>
> The idea of specifying fieldsets as a list of dictionaries makes sense.
> Analogous to how we do field groupings in the existing admin and that
> seems to have worked fairly well.
options? BaseModelForm counts with it - so it is no problem in
BaseForm? If it is not I don't say nothing more on Base- variants.
> I think a lot of your other presentation-controlling changes reduce
> flexibility, though. I like the current Form class design a lot. It
> really does strikea nice balance between default layout and allowing
> lots of stuff to be controlled without having to write lots of code.
> Your changes make this a bit harder (e.g. moving presentation stuff
> further down into the widgets). Is all that really necessary just to
> make fieldsets work? I'd rather see that separated out, since fieldsets
> stirkes me as something that is just grouping existing fields, so
> shouldn't require a lot of changes to how they're displayed (you might
> well want to make such changes, but are they functionally stopping just
> the fieldset changes? If not, they're a separate issue).
1. Fieldsets are much more complex that forms without them - in
metaclasses, output methods and so on. And I am afraid I would not be
able to create the patch without some refactoring.
2. My target was not "to add support of fieldsets to newforms" but "to
add support of fieldsets to newforms which are customizable enough". I
can only repeat example with big form which should be changed only a
little. It seems quite useless to be able to print form with fieldsets
only as {{ form }} - if it is in 50 % situations unusable (it is much
more probable that big forms could be somewhere different). Maybe I
would tell that there are changes which should be done before the
fieldsets support.
My problem is that I dislike writing of html forms (or html generally)
- so I love newforms. I only want to use default variant and to change
something only if someone complains. So I try to ask - what shall I do
when I want to change one row? Or change one fieldset? Or add html
attribute to one row? Or add html attribute to one fieldset? And I
feel that with this patch it would be quite simple.
I think that it is possible to have more presentation control without
reducing of flexibility. But I agree that my patch reduces it
sometimes - I hope I will be able to fix it.
> > Additional options for newforms:
> >http://code.djangoproject.com/ticket/6631
>
> There are a lot of separate things there. Many of them seem to be
> backwards incompatible without introducing new functionality and without
> necessarily making the code much simpler.
>
> If you want to write a custom form, you subclass the Form class, so you
> already have the ability to write your own rendering function and
> control the error class and the label and things like that. The current
> defaults work pretty well, too, for simple stuff.
popular and it seems that many people are not satisfied with defaults.
> This mail's already quite long. I'll address each of the alternatives
> (plus all the other tickets, in turn) in another mail if nobody else
> expresses my opinions first.
any resolution for the first ticket I don't think it is a good idea to
give some time to it.
And thank you very much for your response.