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

Opened 7 years ago

Last modified 7 years ago

WidgetsList should not remove declared widgets from class scope

Reported by: xaka Owned by: chrisz
Priority: normal Milestone: 1.x
Component: TurboGears Version: 1.1 HEAD
Severity: normal Keywords:
Cc:

Description

class NamedFields(WidgetsList):
  username = TextField(...)
  password = TextField(...)
fields = NamedFields()
fields.username
fields.password

Sometimes very convenient and usefull to refer to a specific widget by name.

Because it was declared in class scope i expect to be able to refer to it in my code.

Make sence only for declared widgets but not passed to the constructor.

Fix and test cases attached.

xaka@ubuntu:~/work/turbogears$ svn info
URL: http://svn.turbogears.org/branches/1.1
Revision: 6997
----------------------------------------------------------------------
Ran 440 tests in 30.086s

OK

Attachments

fix_and_test_cases.diff Download (1.5 KB) - added by xaka 7 years ago.

Change History

Changed 7 years ago by xaka

comment:1 Changed 7 years ago by percious

  • Milestone changed from __unclassified__ to 1.1

comment:2 follow-up: ↓ 3 Changed 7 years ago by chrisz

  • Owner set to chrisz
  • Status changed from new to assigned
  • Milestone changed from 1.1 to 1.x

So you're adding a feature by removing one line of code ;-)

I'm not sure whether this line had a purpose? If only to avoid accidentally overriding list methods e.g. by adding a field with the name "append"? But we could inhibit such overriding like this:

                if name in dir(WidgetsList):
                    del class_dict[name]

Also, if we allow attributes for declaratively defined lists, then to be consistent, shouldn't we set them also when the widget list is defined like this:

    fields = WidgetsList(TextField('username', ...), TextField('password', ...))

This could be provided by adding the following code to the WidgetList.__init__ method:

            for arg in args:
                if arg.name not in dir(WidgetsList):
                    setattr(self, arg.name, arg)

However, attributes for widgets appended or extend later to the widget list would still be missing. We could also handle this, but that will get even more complex.

I'd like to get some feedback before changing something here.

comment:3 in reply to: ↑ 2 Changed 7 years ago by xaka

In TG's code you have few places where you subclassing from base types and define yourself attributes without fear to override base attributes. Why? Because you know what are you doing and for what. TG users too know their intention and what they do. If i make the class (WidgetsList? is the class, yea?) and declare some class-scope attributes, perhaps i'll use them in dot-notation (MyClass?.myattr). Also i know that WidgetsList? is based on list and i will not use "bad" names for attributes.

About what i'm? :) As Python's way says: "Keep it simple and elegant". Python's core guys don't worry about inheriting from base classes and possible overriding, they are don't say you "Hey, man, what are you doing? You overrided our attribute!"

TG too must not worry about such things. If i defined something it should remains here for future use. If i override something and error occured - that's my bad, not TG.

That's all about overriding, subclassing and defining. IMHO.

What about "...widgets appended or extend later to the widget list..."? Keep it simle. User just added item to list, nothing more. In that case he just used it as list...list with additional attirbutes.

comment:4 Changed 7 years ago by chrisz

I tend to agree with you. In case of doubt the most simple solution is often the best, which in this case means simply removing the del statement for the attribute.

Maybe we can get a 3rd opinion.

comment:5 Changed 7 years ago by xaka

You about extending object's scope by passed widgets? Make sence, but few problem exists:

  • every widget in list may haven't name (default "widget" will used) and because setattr(self, widget.name, widget) will not work (duplicate names, overriding).
  • much work need to be done: redefine append, insert, remove, etc., i.e. to be in sync

I think removing del statement and nothing more will be right. i.e. just don't touch what user defined and expecting.

comment:6 Changed 7 years ago by chrisz

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

I've asked ChrisA for feedback and he also agreed we should keep the attribute.

ChrisA also suggested to output a warning if a method gets overridden by a widget attribute which is principally a good idea. But we will actually have to go through all the bases to check this, and I don't know if it's really worth the effort. Also, as Pavel indicated, Python usually does not prevent you from shooting yourself in the foot, and overriding an attribute is the default behavior that has to be expected.

So I left it at the most simple solution, the one suggested by Pavel, committed in the TG 1.1 and 1.5 branches. If you disagree, you may reopen for discussion.

Note: See TracTickets for help on using tickets.