Warning: Can't synchronize with repository "(default)" (Unsupported version control system "svn": No module named svn). Look in the Trac log for more information.

Ticket #114 (closed enhancement: wontfix)

Opened 13 years ago

Last modified 12 years ago

Class-style declaration of Forms

Reported by: kevin Owned by: anonymous
Priority: normal Milestone: 0.9
Component: TG Widgets Version:
Severity: normal Keywords:
Cc:

Description (last modified by kevin) (diff)

The votes seem to be strongly in favor of doing class-style declaration of forms eg:

class MyForm?(TableForm?):

name = TextField?(...) age = TextField?(...)

Attachments

classforms.patch Download (2.8 KB) - added by chadwickmorris@… 13 years ago.
Patch to enable declarative-style forms
classforms-bugfix.patch Download (2.9 KB) - added by chadwickmorris@… 13 years ago.
Updated patch to fix bug with classinit execution
classforms-enhance.patch Download (3.7 KB) - added by chadwickmorris@… 13 years ago.
classforms-enhance.2.patch Download (3.7 KB) - added by chadwickmorris@… 13 years ago.
classforms-kg1.patch Download (6.6 KB) - added by KarlGuertin 13 years ago.
Enhanced 1 with updates
classforms-kg1.2.patch Download (3.0 KB) - added by KarlGuertin 13 years ago.
Same as last but actually a clean patch without my extra container crud
classforms-kg3.patch Download (8.9 KB) - added by KarlGuertin 13 years ago.
Class based forms based on Formencode.declarative
classforms-kg4.patch Download (8.9 KB) - added by KarlGuertin 13 years ago.
Updated
classforms-kg5.patch Download (9.0 KB) - added by KarlGuertin 13 years ago.
Number 5, gives you confidence in my abilities, don't it?
classforms-kg6.patch Download (13.1 KB) - added by KarlGuertin 13 years ago.
Now with testing! Sixth time's a charm?

Change History

comment:1 Changed 13 years ago by kevin

  • Description modified (diff)
  • Summary changed from Allow a form to take in its widgets via positional parameters to Class-style declaration of Forms

Changed 13 years ago by chadwickmorris@…

Patch to enable declarative-style forms

comment:2 Changed 13 years ago by chadwickmorris@…

  • Summary changed from Class-style declaration of Forms to [PATCH] Class-style declaration of Forms

Basic patch to allow declarative-style forms like:

class MyForm?(TableForm?):

one_field = TextField?() another_field = TextArea?()

Included the ordering of the widgets to keep the sorting correct. Any widgets passed at form instantiation will be appended to the bottom of the widgets list. Only works for TableForm?. Based largely on SQLObject, as I'm pretty new to the Python black magic of metaclasses and such.

comment:3 Changed 13 years ago by michele

I'm trying to do some funky things with widgets, with this patch will I be able to do something like this?

class MyForm(TableForm):
    name = TextField()
    age = TextField()
    
myform = MyForm()

myform.name.attrs = {...} <-- what I need

comment:4 Changed 13 years ago by chadwickmorris@…

Not right now. The widgets are deleted from the class as they are loaded into the widget list, not to mention that some work needs to be done to make sure that the declared widgets don't inadvertantly override the widget attributes like name, template, validator and such.

I don't see a reason to not allow widget access directly off of the class, so I'll test that out and update the patch.

Changed 13 years ago by chadwickmorris@…

Updated patch to fix bug with classinit execution

comment:5 Changed 13 years ago by kevin

  • Type changed from defect to enhancement

This is generally cool! I didn't expect to get a patch for this one, because it seemed a little hairy. But, you made it look easy. Here's a couple of comments:

  • I don't see anything in itertools.count docs that imply it's threadsafe. I'm pretty sure that we can't count on this code being run in a single thread, so the safe thing to do is to serialize access to the counter.
  • you *could* have the widgets in both the class and the self.widgets list. Since we're talking object references, modifications like Michele's question should work fine
  • one thing that still bothers me is the need to declare the class and then instantiate it for use. this is how forms are fundamentally different beasts from SQLObjects (for example). I'm wondering if it's possible to have the metaclass turn the instance methods into classmethods with a singleton. (don't feel compelled to do that for this patch - we can punt for the moment on this)

comment:6 Changed 13 years ago by KarlGuertin

The answers we seek to declarative form classes are in FormEncode, and in formencode.schema.Schema in particular. I've been working along a similar trajectory since yesterday afternoon trying to create nestable forms (or, more specifically, a container class and a list class that will name-mangle the containing widgets).

In testing this code, I'm getting incorrect behavior (I import widgets as W):

class SiteForm(W.TableForm):
    name = W.TextField()
    role = W.CheckBox()

...
    def test
        return dict(
            data = site,
            form=SiteForm()
            )

produces

<FORM ACTION="/edit" METHOD="post" NAME="form">
    <TABLE BORDER="0">
        <TR>
            <TD>
            </TD>
            <TD>
            <INPUT TYPE="text" NAME="widget" VALUE="">
            </TD>
        </TR>
        <TR>
            <TD>
            </TD>
            <TD>
            <INPUT CHECKED TYPE="checkbox" NAME="widget">
            </TD>
        </TR>
        <TR>
            <TD>
            <LABEL CLASS="formlabel" FOR="role">Role</LABEL>
            </TD>
            <TD>
            <INPUT TYPE="text" NAME="role" VALUE="1">
            </TD>
        </TR>
        <TR>
            <TD>
            <LABEL CLASS="formlabel" FOR="name">Name</LABEL>
            </TD>
            <TD>
            <INPUT TYPE="text" NAME="name" VALUE="fcn_399">
            </TD>
        </TR>
        <TR>
            <TD> </TD>
            <TD>
            <INPUT TYPE="submit" NAME="submit" VALUE="">
            </TD>
        </TR>
    </TABLE>
</FORM>

Changed 13 years ago by chadwickmorris@…

comment:7 Changed 13 years ago by chadwickmorris@…

Added a new patch. This code will make the widget take its name from the attribute name, if another isn't provided. It will also set up the label similarly. The itertools.count() has been changed to iter(xrange(sys.maxint)), which is threadsafe and should be fine as long as less than 2147483647 (or so) widgets are instantiated. You can access the widgets by class.widgetname.attr, but if the widget name collides with the TableForm? class, the widget will be removed from the class attribute. Name mangling is in order there I'd imagine, and I'm open to suggestions on a convention.

KarlGuertin?: The previous patch was very simple - it only populated the widgets list from the class attributes. To make your example work, you would have needed to add name="name" and name="role" to your field declarations. In this new patch, that wouldn't be necessary, but the name field would not be accessible through SiteForm?.name

comment:8 Changed 13 years ago by michele

Great! :-)

Maybe we can move any widget attribute/method to be namespaced with a leading underscore. Removing the widget from the class attribute seems less than ideal to me and yet another thing to document (but that anyway will cause problems).

Kevin?

Changed 13 years ago by chadwickmorris@…

comment:9 Changed 13 years ago by chadwickmorris@…

Simple enough. Leading underscore added.

Changed 13 years ago by KarlGuertin

Enhanced 1 with updates

Changed 13 years ago by KarlGuertin

Same as last but actually a clean patch without my extra container crud

comment:10 Changed 13 years ago by KarlGuertin

The above patch is (besides not working in trac) moves the attrs into a widgetattrs dict, prepends _w_ (prevent namespace collisions with base widget, e.g name) and uses getattr so that they work in the class.

Similar solution as enh2 above but guaranteed not to clobber another attribute.

With enhanced 1 I was getting duplicate fields using the above example, so the constructor ignores added widgets if there are widgets as part of the class declaration. I figure adding widgets at instantiation is deprecated if you're declaring them in class...

comment:11 Changed 13 years ago by kevin

I don't see anything that specifically states xrange is threadsafe, but I'm going to guess that it likely is. (It's written in C and probably has the global interpreter lock anyhow...)

Nice stuff!

comment:12 Changed 13 years ago by kevin

There are two problems with the patch (caught by the tests -- run nosetests to see what I mean):

  • this kind of Form subclass should not "register" (which is how things show up in the widget browser, etc).
  • labeltext added the old way gets clobbered.

I've fixed the first problem, but I haven't yet fixed the second. Once I do, I'll commit.

comment:13 Changed 13 years ago by KarlGuertin

I'm actively working on this and have a few bugfixes/improvements. I'll have a patch up in a short while.

Changed 13 years ago by KarlGuertin

Class based forms based on Formencode.declarative

comment:14 Changed 13 years ago by KarlGuertin

classforms-kg3 is a rewrite of the metaclass stuff. This patch rebases widget on declarative.Declarative because I know that Kevin wants to use classes and instances interchangably, which Bicking does using the Declarative baseclass. This patch as implemented does not handle this (I don't fully grok what's going on there thus I haven't put it in), but the machinery used in SQLObject and FormEncode is there for when that gets done.

As part of the rewrite, I removed MetaWidget? (which has moved to classinit) and turned name into a property so that it automagically propagates to the label. I refactored the functionality of TableForm? into a base Form class (I have a ListForm? ready for another patch that depends on this).

As I'm writing this, I realize I don't have underscore shadowing of the removed attributes, so there will be a quick followup to this (I plan on moving them to a .w sub-'object' to match the magic sqlobject .q). Patch in 10 mins.

Changed 13 years ago by KarlGuertin

Updated

comment:15 Changed 13 years ago by michele

Great, while working on #125 I've also refactored TableForm? to the Form class. I will wait until this ticket gets committed to attach my patch.

While working on this, would be possible to move all the generic stuffs (not related to a Form) to a CompoundWidget? class (that should at least contain retrieve_css and retrieve_javascript as a beggining)?

This way it should be possible to reuse this mechanism for every other widget that needs to embedd others widgets.

Better to ask what Kevin thinks about this.

comment:16 Changed 13 years ago by KarlGuertin

I have a Container class that can takes a list of widgets and name-mangles them so that they can co-exist in a single form. I plan on being able to support MultipleJoin? SQLObject columns using this class.

My goal is to be able to have:

form
  widget1
  widget2
  container1
    container1.widget1
    container1.widget2
  container2
    container2.widget1
    container2.widget2
    container21
       container2.container21.widget1

I planned on leaving retrieve_css and retrieve_js as Form methods with the idea that the form is what has the submit button and thus the retreive methods should be handled there. Does that make sense?

comment:17 Changed 13 years ago by michele

Ideally retrieve_css and retrieve_javascript should work recursively in the right way.

Every widget should have those two methods, if the widget is simple they should simple reuse the Widget implementation, if the widget is a container (CompoundWidget?) the implementation used by the Form should do the trick.

But I haven't tested it.

So, in your example:

form
  widget1
  widget2
  container1
    container1.widget1
    container1.widget2
  container2
    container2.widget1
    container2.widget2
    container21
       container2.container21.widget1

form it's a CompoundWidget?, widget1 and widget2 are simple, container1 is compound and so on.

If we move retrieve_css and retrieve_javascript in a ideal CompoundWidget? base class it should just work I think.

comment:18 Changed 13 years ago by KarlGuertin

Looks like that's correct and I'll add it in, but we should move this discussion into a bug about compound widgets/containers rather than one about class style forms.

comment:19 Changed 13 years ago by michele

Yes, you are right.

Let's make class style forms fisrt.

Changed 13 years ago by KarlGuertin

Number 5, gives you confidence in my abilities, don't it?

comment:20 Changed 13 years ago by KarlGuertin

After a bit more testing I realized that the widgets are coming out backwards and that the label is being shared between classes. This patch addresses both problems. I haven't tested css and javascript, but currently the list is shared between all widgets. I think I've exercised everything else.

comment:21 Changed 13 years ago by michele

Hi Karl,

Great work.

Ok, I know that this discussion regards declarative style forms, anyway my "new" idea is strictly related, I want to throw it there so that Kevin and the others can comment on it, if it sounds right I think it's worth doing it in one shoot while we are at it.

While working on #125 I've seen that for widgets like a form you end up with a least two places that can contain a widget:

  • the widgets attribute
  • the submit attribute

This is also true for simple widgets (like a TextField?) that really shouldn't contain any other widget but can end up with a label widget in the label attribute.

First of all, I plan to provide a patch that removes this and gives the Form full control of the label that should be used for every field (this makes sense since is the Form who is displaying the field and not the field itself, the field should just provide the labeltext).

The other problem is that this unpredictable distribution of widgets on different attributes can make relatively difficult the retrieve_css and retrieve_javascript methods, for example we could use a different submit button that requires some javascript but this javascript actually would not be listed from the above methods.

My idea is that every CompoundWidget? should put it's child widgets inside the widgets attribute and categorize them here.

For a Form for example:

class Form(CompoundWidget):
    widgets   
       |_ fields
       |     |_ TextField
       |     |_ TextArea
       |     |_ ...
       |_ label (widget to use to show a label)
       |_ error (widget to use to show an error)
       |_ submit (widget or widgets to use as submit buttons)

in this way retrieve_css and retrieve_javascript can simply cycle recursively the widgets attribute, when you are making a widget or using it you known that if this is a CompoundWidget? it's child widgets are "all" inside the widgets attribute.

If we move all this logic onto a CompoundWidget? class we can easily have declarative style for every CompoundWidget?, by the way since a normal Widget should not contain any other widget the declarative style it's not needed there.

Probably I'm totally wrong, and if you feel I'm off topic (sorry then) but this makes sense we can move to another ticket the discussion.

comment:22 Changed 13 years ago by kevin

You seem to be heading down a good line, and I hope to get a chance to play with this soon.

Please, though, before submitting a patch *run the tests*. If a test needs changing because of a change in behavior, that's good to know and good to see. It would also be nice to see the behavior exercised in a test.

  • easy_install nose
  • nosetests

I'm not sure if I'll get a chance to play with this today.

comment:23 Changed 13 years ago by kevin

by the way, I should mention that there is currently 1 test that is failing: a widget is not properly rendering its source(). don't worry about that one. there are 3 other tests failing after applying this patch.

Changed 13 years ago by KarlGuertin

Now with testing! Sixth time's a charm?

comment:24 Changed 13 years ago by KarlGuertin

Ah, that's how testing works with nose. I'm getting everything passing except the source (as noted) and always_transaction, which fails when connecting to the db.

The biggest source of tests failing was the call in controllers.py listed above. I wasn't sure why the form is being called, so I just added a test to make sure that the callable isn't, in fact, the constructor. If it is, then that needs to be adjusted.

I also added tests to enforce the classforms contract: widgets appear in the order declared in the class, passed in widgets get appended to the end or override widgets of the same name.

comment:25 Changed 13 years ago by KarlGuertin

A more phiosophical question: If I have a class that takes the same name as one of the widget properties (e.g. name) I can't set both as a class declaration. I can set one and pass in the other but that's awkward.

The question is how to fix it. The rest of the code, as-is, expects the widget attributes to be in their widget places. This is why I move the widgets to the .w. object.

SQLObject handled this by prefixing class names with an underscore and now with sqlmeta. I don't want to do a widgetsmeta because I'll just move the attributes to the main class for compatibility with the rest of the widgets framework and I think that having both widgets and a widgetsmeta swap places would be really weird as a client.

I can copy the old sqlobject method fairly easily, but is that what we want?

comment:26 Changed 13 years ago by kevin

Now THAT'S a primo quality patch! Tests that run and new tests to boot.

I did need to change the handling of "register". It may have seemed kind of goofy the way it was working, but there was a reason for it: you don't want to inherit the "register" class attribute. As the patch stood, TableForm? was not getting registered, because Form (correctly) had register=False. This was made extra tricky, because declarative-style forms should not be registered... just legitimate widgets that are intended to be used as such.

Anyhow, that was a fairly easy fix, compared to the total of a 13K patch!

This is committed in [437]. Thanks!

comment:27 Changed 13 years ago by kevin

Now - on to the question in Karl's last comment. That is a tricky problem. What do you think of:

class MyForm?(TableForm?):

class w:

name = TextField?() age = TextField?()

This style is less pleasant for the user. More pleasant for a widget-writer. That means it's probably wrong :)

It's a shame for all widgets to have to jump through contortions just for declarative forms. What if we create a widgetmeta class that contains the information and give the base widget class a getattr to pull things from there? The Forms are then free to have whatever name they want attached to the declarative style.

I should say that, on the whole, I think the declarative style forms are more trouble than they're worth. But, I also think that the machinery is almost there...

comment:28 Changed 13 years ago by michele

I think we should aim at the style that's more pleasant for the final widget user. IMHO that's also one of the reasons behind RoR succes (syntax sugar) and something TG should also consider (0.9 seems to be on the right track ;-)).

So +1 for the widgetmeta or whatever solution would do the trick.

Or what about (like Kevin's suggestion combined with my last comment):

class MyForm(Form):
   class fields:
      age = ...
      name = ...

   class submits: (no need for the SumbitList widget)
      save = ...
      discard = ...

   label = ... 

Not that bad.

comment:29 Changed 13 years ago by kevin

Syntax sugar is good on the one hand, but it's unpythonic. "Explicit is better than implicit". *Too* much magic makes it hard for users of the framework to debug problems. In this particular case, though, I don't think we've gone too far with the magic.

I think I'd prefer saving the widget user from typing the extra embedded class. Also, I don't think submit buttons need special handling if you have more than one: they're just regular widgets that whose values get passed in.

Something that did just occur to me, though, is that there *would* be reserved words like "insert" and "render", because these are still methods on the widget. Perhaps it really is cleaner to have the user use an embedded class. Kid is fighting with this right now, because you can't use variable names that match methods in BaseTemplate? (like "content").

comment:30 Changed 13 years ago by michele

Agreed regarding magic vs explicit, that's also the reason I prefer python vs ruby. :-)

And yes, reserved words are annoying and they leads to unexpected behaviors (like for Kid).

My second comment regarding the leading underscore was really intended for this issue and not for the widgets attribute, I can see only two solutions:

  • The simplest (that will not work for fields with a leading underscore), a leading underscore for every attribute/method in the Widget class:
    class Widget:
       _template
       _validator
       ...
       _insert()
       ...
    
  • An embedded class (where I would use the fields name)

Both seems not the ideal, this is also the sign IMHO that classes are not meant to be used that way, but I must admit that a declarative form is easier to declare and understand and looks better (but not the fact that you need to declare and then instantiate it).

Others ideas? The embedded class seems the only reasonable solution.

comment:31 Changed 13 years ago by kevin

  • Summary changed from [PATCH] Class-style declaration of Forms to Class-style declaration of Forms

I'm taking this off the pending patches list for now, as the patch has been applied and we just need to decide how to finish this. I'm going to raise a discussion on the list.

comment:32 Changed 13 years ago by michele

  • Status changed from new to closed
  • Resolution set to wontfix

Closing this as we decided to go with another approch.

See #454.

Note: See TracTickets for help on using tickets.