On 12/22/05, [EMAIL PROTECTED] <[EMAIL PROTECTED]> wrote: > > After following the form and widget threads for a while, I finally did > a little experimental hacking today, and found a few little itches -- > that is, small annoyances that can be solved with a little localized > application of force. :)
A little localized application of force is fine, and I'm sure there are *many* nits with the current implementation. My concern is to make sure there's no glaring "structural" errors. I wouldn't want to create the Tacoma Narrows Widgets: http://www.enm.bris.ac.uk/research/nonlinear/tacoma/tacoma.html > Here are the things that bugged me today -- > > * Controllers want a handle into cherrypy.request. The base controller > class ought to grab one, so that any controller method can see the > request in self.request. This helps with unit testing (no need to mock > a cherrypy module just to mock the request) -- and generally, it makes > sense to me to put things near where they are going to be used. I don't really mock the cherrypy package for testing. I just toss in a fake request. Adding a self.request reference to the controller seems reasonable for convenience's sake. Can you open a ticket for that? (http://trac.turbogears.org) > > * If there are validation errors, _execute_func (in > turbogears/controllers.py -- part of the implementation of expose) > should pass them to the function it's executing. Having to go out and > find them via cherrypy.request (or even self.request) is bad for a > number of reasons. It makes isolated testing more difficult: you can't > test your method's error handling just by passing errors to it, you > have to set up some other object with a property with a magic name. It > obscures the flow of data, and makes the code harder to read. Much > better, IMO, would be to have _execute_func add (say) validation_errors > to the call to func() when (and only when) errors exist. The problem with this is that all of your functions would either have to have **kw or specifically have a validation_errors parameter. Other ideas that have been discussed are to call another method (if available) to handle errors. (yourmethodname_error). We can also add a property for self.form_errors, if that makes people happy. > * TableForm... isn't. It's not a form, or a table, that is, but a > layout widget. It is a container for other widgets that draws them > inside of a particular view. I think it needs a rename and a bit of > refactoring to express more clearly the flexibility that it has. Agreed, to a point. It's always been my intention to pull the bulk of the code out of that class... but, in the end, it's a widget that renders out a form that is laid out table-style. Putting "Widget" in all of the widget names would be redundant, since they're already in the widget package. Thus, TableForm is still a fine name for the final object that gives you a form with the fields laid out in a table... but the code should definitely be pulled out of there and put in a better place. > In case these things are troubling you all too, I'll add tickets and > submit patches for each as soon as I have net access from the same > machine as I have access to my code. That will probably be a few days > from now. Ah, vacation! :) Tickets are cool. patches even better! Thanks! (And thanks for nose! it keeps getting better and nicer to use...) Kevin
