> It definitely needs some discussion, so please -- ask, discuss, criticize, > share some thoughts and one day, hopefully +1. :)
You won't get any argument from me on this one.
In my ideal world, Django's admin should just be a light customization of Django's native form capabilities, tied to some nifty model introspection logic. However, as currently implemented, there are plenty of features -- like fieldsets -- that are only available to admin forms. Refactoring this code out of admin, and into base forms is certainly something worth pursuing.
Also worth keeping in mind is the intersection of this sort of refactoring with efforts to replace Django's form rendering with a template-based renderer. This was the subject of a Google Summer of Code project this year, and while it hasn't been merged to trunk, there's some obvious potential for overlap with any plans to introduce fieldsets into base forms.
From an quick inspection of your patch, you look like your on the right track -- you've certainly got all the right pieces (tests, docs, and what appears to be a solid initial implementation).
The only thing that I would like to see is an attempt to eat our own dogfood -- i.e., to try and replace the admin's custom fieldset tools with this new capabilitiy. Dogfooding is an important step in demonstrating that you've got the API right; given that this is something that is being factored out of admin, actually using it in admin would be a good proof of that concept.
My patch obviously was not posted to be merged into trunk in it's current state.
It is meant to be a message "i'd like to do that, seriously". Now that I see general
community support I can perfect it. There several more things I'd like to do, mainly:
1. "Dogfooding" (which is a great phrase I've never heard of :) my concept by
2. `classes` option, just like the one in current admin fieldsets;
3. Several fields in one row, once again like in current admin;
4. Write some more documentation in my poor-English and ask someone nicely
to translate it to real-English
This features should be complete by the end of the week, and then we'll see.
PS. If there's anyone willing to edit some really bad writing, please contact me.
2011/11/28 Mikołaj Siedlarek <mikos...@gmail.com>:
> Hi Russel, > My patch obviously was not posted to be merged into trunk in it's current > state. > It is meant to be a message "i'd like to do that, seriously". Now that I see > general > community support I can perfect it. There several more things I'd like to > do, mainly:
> "Dogfooding" (which is a great phrase I've never heard of :) my concept by > modyfying admin;
Apologies for the confusion. For those not familiar with the term:
> `classes` option, just like the one in current admin fieldsets; > Several fields in one row, once again like in current admin; > Write some more documentation in my poor-English and ask someone nicely > to translate it to real-English
Don't worry too much about this bit. The hard part of writing documentation is always the first draft, and coming up with a good set of examples to use in that draft. If you can get to a first draft that is roughly understandable (and the first draft you've got in your patch is certainly good enough), it can get cleaned up on editing when it is committed.
I am a bit skeptical about defining form fieldsets in Python code, as they have no effect on server-side interpretation or validation of form data, they are purely an HTML/presentational issue. I think that in general we should be moving form presentation entirely into templates rather than adding more presentation-related attributes into Python code. There's already work begun on this via a Google Summer of Code project (see #15667 and #16922); there are some barriers to overcome (particularly performance issues with the template engine), but I think it's still the right approach for the long term. And the more form-presentation controls we add in Python code, the more difficult this transition becomes (because we end up with two conflicting methods of defining a forms layout).
However, I can see some justification for simple fieldset definitions in the form definition, as a high-level way to group fields. This can allow more flexibility in defining custom reusable form layouts (via templates) that depend on having such field groupings. And it's clear that others want this feature: I won't stand in the way of it if another core developer wants to champion its inclusion.
However, I feel quite strongly that even if we add this, we should NOT add the additional "classes" and "fields in a row" features that you mention. Fieldsets themselves are defensible as logical "structure" of the form - these additional features are getting into the arena of trying to fully define HTML layout of a form in Python code, which I think is fundamentally the wrong approach, and makes it more difficult for non-Python-programmer template authors to work with Django forms. Why should forms be an exception to the general rule that HTML and presentation belong in templates?
I see the admin as the exception here, rather than as a model for how the forms library should work. The admin's purpose is to create generic customizable CRUD screens for arbitrary models; this is necessarily going to involve defining the available axes of presentation customization, and making it easy to do those particular customizations. That's a very specific use case; it means trading off flexibility for convenience in very simple customizations. That's an appropriate tradeoff for the admin to make in building on top of django.forms; it's not an appropriate tradeoff for django.forms, IMO.
In sum: +0 to fieldsets in Python code, -1 to classes and rows defined in Python code.
To be honest, I think that Django will never achieve complete separation in terms of MVC
model staying this convenient tool. There are many places where Django has a simple way
of doing things, even when it's not full MVC-compilant. Current Form.as_(table|ul|p) methods
are examples. I believe that Django should keep simple things simple.
Template-based forms are great idea (however can't say I've read the whole patch, it's rather
huge), but I don't think that creating template to separate groups of fields is keeping simple
Fair point about `classes` and several fields in a row though. I think you're right and decided
not to do that.