Kevin Dangoor wrote:
> Before I get to my main point, I'd like to mention something I just
> noticed: my preference is to have "name" be the first parameter. Like
> this:
>
> def __init__(self, name=None, rows=7, cols=50, **params):
> super(TextArea, self).__init__(name, **params)
>
> That way, widget users can always do this:
>
> TableForm(widgets = [TextAreaField("foo")])
>
> Is that a bad idea? It's easy to test for.
Just a matter of style... I personally favour named arguments, though
more verbose, for something like this, where we have multiple levels of
inheritance and parameters accepted at various layers.
Anyway, what it's absolutely clear is that we need some consensus as to
avoid the kind of bugs fixed at r660. :)
I also think we should apply Michele's suggestion (#490, try finding
"CalendarDatePicker" at the page if you don't want to get booored... ;)
of listing at the constructor of our Widget subclasses all the
parameters of our superclass (plus the new ones we are taking) to avoid
**kw abuse (and possible bugs). Though if were subclassing more than
one superclass this might not apply... (as most of the widgets at
forms.py which subclass FormWidget and SimpleWidget).
Example (from r694):
class Widget(object):
def __init__self, name=None, template=None, default=None,
validator=None):
class SimpleWidget(Widget):
def __init__(self, name=None, template=None, default=None,
validator=None,
attrs=None, **kw):
super(SimpleWidget, self).__init__(
name, template, default, validator, **kw)
This way it's easier to spot which parameters we have available for
override or to initialize or custom widget.
> Now to my main point: the widget above is the way it looks after a
> change I made. Previously, it didn't have the template_vars and had an
> update_data that looked something like this:
>
> attrs = d["attrs"]
> d["rows"] = d[attrs].get(rows, self.rows)
> d["cols"] = d[attrs].get(cols, self.cols)
>
> My preference, as you can see above, is for attributes that are
> specifically supported in the constructor to be explicitly contained
> in the template (as they) and not rely on attrs. With the style that's
> committed now, a user can do this:
>
> tf = TextField(rows=5, cols=10)
> tf.display(rows=8, cols=39)
> or even
> tf.display(attrs=dict(rows=10, cols=992))
>
> So, this style is a bit less code (it leverages template_vars), it's
> more explicit in the template and it still makes it easy (and
> obvious!) for a user to override.
>
Agree, this way you don't need to traverse __init__ and update_data at
all the levels of inheritance to see where variables are "picked up"
for insertion in the template.
For example, if you see template_vars = ["name"] at Widget, you'll know
that everything that inherits from Widget (all widgets ;) will have
their name available at the template, same goes for "field_class" and
"field_id" at FormField, etc... You also know that it's the item of
least preference (can be overriden at the constructor OR at display
time). Standard behaviour for all widgets and no need to read
update_data (from potentially a couple of base classes) to see in which
order they are inserted at the template... (well, you still have to
read "template_vars", but that's better than following various ifs,
elifs and elses ;)
my 2 cents,
Regards,
Alberto
