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.
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).
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.
This one should be brought up on this mailing list. It's not appropriate
for a ticket.
If Barbie is so popular, why do you have to buy her friends?