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 #419 (closed enhancement: fixed)

Opened 13 years ago

Last modified 12 years ago

Allow form widget to accept FormEncode Schema

Reported by: randall@… Owned by: anonymous
Priority: normal Milestone: 0.9
Component: TG Widgets Version:
Severity: trivial Keywords:
Cc:

Attachments

sugar.patch Download (5.5 KB) - added by alberto 13 years ago.
schema.patch Download (4.2 KB) - added by anonymous 13 years ago.
schema.2.patch Download (5.1 KB) - added by alberto 13 years ago.
proposal POC

Change History

comment:1 Changed 13 years ago by anonymous

  • Component changed from TurboGears to Widgets

comment:2 Changed 13 years ago by michele

[Disclaimer: I'm not a formencode expert]

Anyway I think this could be done, we can let specify a validator on the form constructor (a thing you can already do but has no effect) and then on the validate method we can do this:

  • if the form has a validator defined (and this validator should clearly be a FormEncode Schema, we need a check here to avoid wrong things) we can use it to validate our values and avoid to pass the validation along every form field
  • if the form hasn't a validator defined we to what we are doing actually

Could this work? it doesn't even sound that difficult we just need to modify the validate method a little to check if there is a form validator and then use it or instead use the normal procedure.

comment:3 Changed 13 years ago by alberto

  • Priority changed from low to high

comment:4 Changed 13 years ago by alberto

would be nice to to use chained validators

comment:5 Changed 13 years ago by alberto

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

I've committed at r738 a Compundwidget customization which allows this. See included tests as means of documentation (for now). Seems nothing is broken... Some internal function renames so please feel free to reopen and comment on this. I've added this functionallity at CompundWidget? as I think this is something that might apply to all Compounds (FieldSets? included), maybe it's not appropiate so comments please :)

Alberto

comment:6 Changed 13 years ago by michele

I think it's very good. :-) And yes it just belongs to a CompoundWidget?. Great work.

comment:7 Changed 13 years ago by alberto

  • Priority changed from high to normal

I think some typing can be saved by some metaclass magic and integrate a schema validator into a WidgetsDeclaration?. It would increase coupling between widgets and formencode but i think it shouldn't be much of a problem either.

class TestSchemaValidation:
    class Fields(widgets.WidgetsDeclaration):
        name = widgets.TextField()
        age = widgets.TextField()
        passwd = widgets.PasswordField()
        passwd2 = widgets.PasswordField()

    class FieldsSchema(formencode.schema.Schema):
        chained_validators = [validators.FieldsMatch('passwd', 'passwd2')]

        name = validators.UnicodeString()
        age = validators.Int()
        passwd = validators.NotEmpty()
        passwd2 = validators.UnicodeString()

(from http://trac.turbogears.org/turbogears/browser/trunk/turbogears/widgets/tests/test_widgets.py?rev=738

into something, IMO, nicer and more compact:

}}} #!py

class Fields(widgets.WidgetsDeclaration?):

class Schema:

chained_validators = [validators.FieldsMatch?('passwd', 'passwd2')]

name = validators.UnicodeString?() age = validators.Int() passwd = validators.NotEmpty?() passwd2 = validators.UnicodeString?()

name = widgets.TextField?() age = widgets.TextField?() passwd = widgets.PasswordField?() passwd2 = widgets.PasswordField?()

}}} or even:

class Fields(widgets.WidgetsDeclaration):
    class Schema:
        chained_validators = [validators.FieldsMatch('passwd', 'passwd2')]
    name = widgets.TextField(validator=validators.UnicodeString())
    age = widgets.TextField(validator=validators.Int())    
    passwd = widgets.PasswordField(validator=validators.NotEmpty())
    passwd2 = widgets.PasswordField(validator=validators.UnicodeString())

I think it shouldn't have to break existing code in any way, just syntactic sugar.

Opinions?

comment:8 Changed 13 years ago by anonymous

  • Status changed from closed to reopened
  • Resolution fixed deleted

comment:9 Changed 13 years ago by anonymous

yikes! should better preview next time... sorry

comment:10 Changed 13 years ago by godoy

Third syntax looks better to me because it is more similar to what we have now -- declaring validators attribute for each widget -- and avoids code redundancy. The added bonus is then having the chained validator, what is GREAT IMHO! :-)

My vote goes for this third syntax:

class Fields(widgets.WidgetsDeclaration):
    class Schema:
        chained_validators = [validators.FieldsMatch('passwd', 'passwd2')]
    name = widgets.TextField(validator=validators.UnicodeString())
    age = widgets.TextField(validator=validators.Int())    
    passwd = widgets.PasswordField(validator=validators.NotEmpty())
    passwd2 = widgets.PasswordField(validator=validators.UnicodeString())

comment:11 Changed 13 years ago by michele

I'm not so convinced, I don't see the need to touch WidgetsDeclaration? because that's just a Widgets declaration not a Schema declaration.

A WidgetsDeclaration? instance provides you a list (in the python sense) of widgets, I think it helped to remove a lot of magic from the previous mechanism, but a inner class just adds back some magic inevitably.

Where is the difference with this?

class UserFields(widgets.WidgetsDeclaration):
    name = widgets.TextField(validator=validators.UnicodeString())
    age = widgets.TextField(validator=validators.Int())    
    passwd = widgets.PasswordField(validator=validators.NotEmpty())
    passwd2 = widgets.PasswordField(validator=validators.UnicodeString())

class UserSchema(formencode.schema.Schema):
    chained_validators = [validators.FieldsMatch('passwd', 'passwd2')]

form = TableForm(fields=UserFields(), validator=UserSchema)

I feel it as more natural, inner classes are too much magic.

Also as I said WidgetsDeclaration? is used to just dump a list to the fields parameter of a Form, if we want to also use it to provide a validator for the form we will need some magic-logic on the CompoundWidget? side to manage this thing, the big aspect of WidgetsDeclaration? is that it's totally transparent to the Widget itself (ok, apart for the declarative_counter but that's just an attribute containing a number).

Example:

class Fields(widgets.WidgetsDeclaration):
    class Schema:
        chained_validators = [validators.FieldsMatch('passwd', 'passwd2')]
    name = widgets.TextField(validator=validators.UnicodeString())
    age = widgets.TextField(validator=validators.Int())    
    passwd = widgets.PasswordField(validator=validators.NotEmpty())
    passwd2 = widgets.PasswordField(validator=validators.UnicodeString())

form = TableForm(fields=Fields()) <- and the who passes the validator?! O_o

Ok, maybe I feel to much involved in keeping WidgetsDeclaration? as simple as possible. :D

comment:12 Changed 13 years ago by godoy

Good point Michele. Simple is better. I liked the syntax, but then mixing validation inside forms declaration and also on schema declaration isn't confusing? As I see, schema would be used just for more complex validation -- such as the one that involves two fields above -- while the form itself would contain validators for each individual field... It fits my brain if I got it correctly.

comment:13 Changed 13 years ago by alberto

  • Summary changed from Allow form widget to accept FormEncode Schema to [PATCH] Allow form widget to accept FormEncode Schema

Hi Michele,

I was hacking on it when I first read your comment but I just couldn't stop my metaclass push-ups :-). I really think it's not that complicated, WidgetsDeclaration?'s new method get's 3 extra lines + 1 extra attribute (so Form can check for it later). The rest of the job it's done at Form. I agree that it might be a little magical, but magic is sometimes good... :D

I understand your point on keeping WidgetsDeclaration? simple, maybe it's not here where it belongs... but I could change that if you point me in the right direction.

I'd really would like to have some way to keep the DRY principle and not repeating so many lines of code. I'm open to any suggestion.

One big advantage of this system is that it's 100% backwards compatible with the current system AND takes advantage of chained_validators and all those formencode goodies.

Now I realllly need to go. I'm already late and my friends are gonna kill me. Code explains me better than word, so here's a patch see what you think...

Regards, Alberto

Changed 13 years ago by alberto

comment:14 Changed 13 years ago by michele

Hi Alberto,

well the problem is that WidgetsDeclaration? as I said has been made to be totally transparent to the Form class, in this way it's not transparent any more.

What I don't like from your example (looking at the test) is the magic going on:

  • with the inner Schema class
  • with this form = widgets.TableForm?(fields=fields) where in the fields parameter ends up a validator

Now looking at it from a DRY POV, I will take your example:

class Fields(widgets.WidgetsDeclaration): 
    class Schema:
       chained_validators = [validators.FieldsMatch('passwd', 'passwd2')
    name = widgets.TextField(validator = validators.UnicodeString())
    age = widgets.TextField(validator = validators.Int())
    passwd = widgets.PasswordField(validator = validators.NotEmpty())
    passwd2 = widgets.PasswordField(validator = validators.UnicodeString())

form = TableForm(fields=Fields())

Total lines: 8

class Fields(widgets.WidgetsDeclaration): 
    name = widgets.TextField(validator = validators.UnicodeString())
    age = widgets.TextField(validator = validators.Int())
    passwd = widgets.PasswordField(validator = validators.NotEmpty())
    passwd2 = widgets.PasswordField(validator = validators.UnicodeString())

class UserSchema(formencode.schema.Schema):
    chained_validators = [validators.FieldsMatch('passwd', 'passwd2')]

form = TableForm(fields=UserFields(), validator=UserSchema())

Total lines: 8

The second example is more explicit (as per zen of python) and uses the same number of lines, the behavior is predicable since you are explicitly declaring that the form also uses a schema validator and you are declaring it in the validator parameter where it belongs, not in the fields one.

I dislike magic in general, and in this case I also think that it doesn't give us any advantage even.

I'm sorry (really) but I'm -1 on this. :_(

Anyway I'm not the one who should decide, so we should hear Kevin opinion.

comment:15 Changed 13 years ago by michele

The bottom line is that I don't see any need to add this code (and magic) just to avoid typing validator=Schema, that's also the right thing to do.

comment:16 Changed 13 years ago by alberto

Hi Michele,

The problem with your example is that it would not work with the way CompoundWidget? is implemented right now :( (please try as i'm a friend's laptop right now so I can only be 99% sure ;)

Once a CompoundWidget? gets a Schema subclass instance as a validator, validate will override the independent validators from each of it's child widget and use the schema. So, unless a better way to attach a Schema validator to a form is found there IS an advantage to this approach:

  • Being able to use chained_validators + all formencode's Schema funonality to address the complete form as a whole.
  • DRYness + less typing

I agree that it might not be the best way to do it, and i'm open to any suggestion on how to design/implement better this functionallity.

There are even some things that must be addressed too, like extending a WidgetsDeclaration? list at runtime.

widgets + [Textfied("bla", validator=v)]. 
}}

Which probably means this is not the best place to hack this in.

My point is that declarative-style forms WITH Schema validation is a Good Thing. This patch is just an idea on how it might be accomplished.

Regards,
Alberto.

P.S just loving this kind of constructive criticism :) Gotta go or i'll get no more wine :D

comment:17 Changed 13 years ago by alberto

P.S Just loving this kind of constructive critism :)

Regards, Alberto

P.P.S I really don't know why but this stupid Win IE6 skipped this part of of the post, either that or this Ribera red wine :D

comment:18 Changed 13 years ago by michele

Hi Alberto,

I also love constructive criticism. ;-)

Yes, there is this problem actually but it can be fixed by using only a part of you patch since you are doing this on the Form:

if hasattr(fields, 'Schema'): 
    self.validator = fields.Schema() 

once fixed the base.py CompoundWidget? to manage this it's not any different than:

form = TableForm(fields=UserFields(), validator=UserSchema())

and again that's IMHO more explicit and just 20 more characters.

I don't see a DRY problem here, I can see a too much magic problem if we *break* the rule that a Widget validator is declared using the validator parameter, here you want to declare it using the fields parameter for a Form and the validator for Fields, why? it's inconsistent.

Regarding the extension of a WidgetDeclaration? at runtime, what do you mean? An instance of WidgetDeclaration? is just a python list and you can do anything you can do with a list:

>>> from turbogears import widgets as w
>>> from turbogears.widgets import WidgetsDeclaration as WD
>>> class CommentFields(WD):
...     name = w.TextField()
...     comment = w.TextArea()
...
>>> class OtherFields(WD):
...     special = w.TextArea()
...
>>> fields = CommentFields() + OtherFields()
>>> for field in fields:
...     print field.name
...
name
comment
special
>>> fields.append(w.CheckBox(name="jorge"))
>>> for field in fields:
...     print field.name
...
name
comment
special
jorge 

That's also one of the reason I'm against this change, I would also like to remove the trailing declared_widget attribute that's still hanging around after the metaclass magic.

comment:19 Changed 13 years ago by alberto

  • Summary changed from [PATCH] Allow form widget to accept FormEncode Schema to Allow form widget to accept FormEncode Schema

Now I see what you mean... you're right! I'll try to post something tomorrow which would make:

!py

class Fields(widgets.WidgetsDeclaration): 
    name = widgets.TextField(validator = validators.UnicodeString())
    age = widgets.TextField(validator = validators.Int())
    passwd = widgets.PasswordField(validator = validators.NotEmpty())
    passwd2 = widgets.PasswordField(validator = validators.UnicodeString())

class UserSchema(formencode.schema.Schema):
    chained_validators = [validators.FieldsMatch('passwd', 'passwd2')]

form = TableForm(fields=UserFields(), validator=UserSchema())

possible my throwing all that evil magic (but good exercise :) metaclass stuff to /dev/a_la_mierda and hack on CompundWidget? to first validate using the field's validators and then using the Schema (over the resulting dictionary (or viceversa, i'll see...).

That way all the logic stays at the CompoundWidget? instance, which is where it really belongs and we don't break the "validaor=V" semantics. right?

Thanks! Helps to hear the opinion of the creatures father ;)

Alberto

comment:20 Changed 13 years ago by michele

Yes, all right Alberto. ;)

Thanks for this civil discussion, and this (/dev/a_la_mierda) touch of *magic* that's not so different from the Italian version. :D

comment:21 Changed 13 years ago by alberto

Hi,

Well, heres the final patch I think... Things are always simpler than they first seem (like most things in life) :)

Proposal:

As probably 99% of the Schema validators which are going to be used are going to have an allow_extra_fields = True parameter (as things will probably not work then unless all the validators are defined at the Schema instance for every possible field)

Shall we have a turbogears.validators.Schema subclass of formencode's Schema that adds this attribute automatically:

at turbogears.validators.py

class Schema(formencode.schema.Schema):
    allow_extra_fields = True

???

I think we are going to save many posts at the ML asking why their schema validators are failing... This way we can even skip the formencode import @ base.py.

Regards, Alberto

Changed 13 years ago by anonymous

Changed 13 years ago by alberto

proposal POC

comment:22 Changed 13 years ago by michele

Actually I never used a formencode Schema, anyway if that's the case it seems a good idea to me.

The patch seems very good. ;-)

comment:23 Changed 13 years ago by michele

Looking at your schema2.patch I actually like your idea a lot, I can't see any drawback.

comment:24 Changed 13 years ago by alberto

  • Status changed from reopened to closed
  • Resolution set to fixed

committed a polished version of 2 at r746

comment:25 Changed 13 years ago by alberto

Mmmmm, I'm beginning to see a really neat way for creating web forms :P Something should be written at the wiki, with some good examples and "best practices" so people start using en masse (and further testing) this whole widget/form/validation framework... maybe I start doing something after lunch. What would really help is a complete tutorial which covered the whole process:

  • form declaration
  • form display
  • controller for receiving data
  • form error handling
  • databse insertion/update of data

I'm not very familiar yet with the new error handling mechanics introduced recently by Simon so I'll leave that blank for someone to fill up later. Collaborations are always welcome :)

comment:26 Changed 13 years ago by michele

Great! :-)

Yes, the widget/form/validation framework now provides a really neat and straightforward way of preparing and managing web forms, it's also too easy probably. :D

I had the same idea as you of putting a nice tutorial on wiki that shows how you should use this new stuffs in a nice way, but I don't have the time ATM. :(

I will attach here a small project that shows how you can use the error handling stuff.

comment:27 Changed 13 years ago by randall@…

I just did a simple password FieldsMatch? test using a FormEncode Schema on a TableForm? and it worked as expected. Thank you for this nice addition.

Randall

Note: See TracTickets for help on using tickets.