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

Opened 13 years ago

Last modified 12 years ago

[PROPOSAL] More boilerplate code reduction at the widgets

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

Description

Hi,

template_vars has reduced the boiler code at update_data considerably as _update_from_self populates the dict sent to the template with default values picked from the widgets instance. I think we can extend this further by reducing the boiler plate at the constructor too. Example:

    def __init__(self, fields=None, show_actions=True, show_add_link=True,
        add_link_title="Add a record", delete_link_msg="Are you shure you "
        "want to delete this?", **kw):

        super(FastDataGrid, self).__init__([], **kw)
        self.fields = fields
        self.show_actions = show_actions
        self.show_add_link = show_add_link
        self.add_link_title = add_link_title
        self.delete_link_msg = delete_link_msg

this is from FastDataController?. As you see, all that this method does is set some instance variables picked up from the constructor args. This is fairly common at all the widget constructors. My idea is to factor this out into the Widget class and have a method that prepopulates the instance with all the variables listed at "template_vars" from the kw arguments (expept "name" as it is a "special" variable which is the only one that, for convenience, can/should be passed as a positional argument).

I think it will also add more consistency to the way widget's are declared.

If special customization needs to be done at the constructor, the self instance can always be rebound, example:

template_vars = ["foo", "bar"]
def __init__(self, name=None, **kw):
    super(MyWidget, self).__init__(name, **kw)
    # self.foo and self.bar have already been bound by our widgets superclass constructor
    self.foo = self.foo + self.bar

I'm going to work on a POC and post here as a patch, just to express myself better :) This shouldn't break the existing API, would just make some code at some constructors unneccesary but that can always be optimized later...

Regards, Alberto

Attachments

boiler.patch Download (3.3 KB) - added by alberto 13 years ago.
boiler.2.patch Download (7.6 KB) - added by alberto 13 years ago.
big_widgets simplified to take advantage of the patch

Change History

Changed 13 years ago by alberto

comment:1 Changed 13 years ago by alberto

There's the patch. I've modified FastDataGrid? to leverage the proposed functionality. IMO it has the following advantages:

  1. Makes sure we can always override all template_vars when subclassing, initializing and at display(): more consistent
  2. Less code to write

Maybe this has a nasty side-effect which I have not thought of so opinions are welcome. If approved I'll write some unittests to make sure it doesn't break in the future and "port" the rest of the widgets to take advantage of this.

Changed 13 years ago by alberto

big_widgets simplified to take advantage of the patch

comment:2 Changed 13 years ago by kevin

The only drawback to this is something that I haven't implemented: automatic documentation in the Widget Browser about the variables available for the widget. That was going to work based on the keyword arguments to the constructor and the PythonDoc? tags that refer to them.

That said, it's possible that this patch won't really conflict with this vaporware feature. So, I'd say go ahead and do it. Boilerplate reduction is a positive thing.

comment:3 Changed 13 years ago by alberto

Well, the variables available to the template can always be extracted from template_vars and the variables available to the widget (which are not template_vars) still need to be passed to the constructor... so the vaporware can still work :)

further testing and committing soon...

Alberto

comment:4 Changed 13 years ago by michele

Seems a very nice addition that makes things even easier and template_vars even more useful as it lists pretty much everything that ends up in the template.

Great work Alberto! :-)

comment:5 Changed 13 years ago by alberto

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

Committed with unitetests at r779

comment:6 Changed 13 years ago by michele

It would be nice if usgin template_vars we can take also of mutable arguments (list and dict) to do the right thing (like a comment you putted on the code says Alberto), for example for doing something similar to what we do with attrs.

The precedence we should be: ctor -> init -> update_data

For dict we update it in this sequence, for a list it's going to be replaced in this sequence.

Opinions?

Also, am I wrong or the patch you committed doesn't yet port all widgets over to this new thing?

Thanks again.

comment:7 Changed 13 years ago by alberto

Yeah, that's a good idea, I implemented something which took care of it but I discarded it... don't remember exactly why :/, I think it had something to do with deciding which was the desired behaviour for lists... or maybe which was the precedence for dict updates... well, you've already cleared this points out so it shouldn't be a problem now ;).

BTW, what is the difference between ctor. and init?? i've been using them interchangeably till now... probably confusing anyone reading my commit messages :/

Yes, I'm pretty sure I've ported all the "portable" widgets.... which one are you referring to? (maybe i've skipped it)

Regards, Alberto

comment:8 Changed 13 years ago by michele

I'm refering to TextArea? probably.

Well, what's the difference? I seen ctor used by you for the first time, anyway.

I think the preference should be:

class attribute (class declaration) -> instance attribute (passed at init) -> update_data

But here we should hear kevin opinion, maybe it doesn't make sense. :D

comment:9 Changed 13 years ago by alberto

TextArea?: You're right! ;), commiting right now...

It makes absolute sense to me (that order of preference) as it's just the order of preference immutables are taking now... the only doubt I think I had was what to do with lists, now I see they should just be replaced.

I'll try to get a patch this afternoon along with the nested variable stuff @ #578,

alberto

comment:10 Changed 13 years ago by michele

Great work Alberto, yep lists should just be replaced as they can't be updated (as we do with css_classes now), the important thing is to not tuch self. with mutables but you already know that.

Now I'm going to study (and probably playing football later today, so that someone doesn't start seeking me, and someone other wondering if I'm male or female like last week! :D)

Ciao!

comment:11 Changed 13 years ago by alberto

I've committed at r783 a polished version of Widget that takes care of this:

I've factored this stuff out to update_data (away from _update_from_self) so it's easier to override the display method (and to better separate logic). One side effect of this is that update_data should be called cooperatively from every widget (DataGrid wasn't doing it) if we want Widget to copy the template_vars for us. I don't see any problem with this, even an advantage if we don't want this variable copying, for any reason, happening in a subclass.

All tests are passing so I guess I didn't break anything

Note: See TracTickets for help on using tickets.