milibopp / obsub

Small python module that implements the observer pattern via a decorator.
Other
12 stars 3 forks source link

' ' + function.__name__ is dangerous #48

Open DanielSank opened 10 years ago

DanielSank commented 10 years ago

Line 171 of obsub.py.

There is little guarantee that someone else won't try to use that attribute name for their own purposes. I would suggest at least using a better mangling character other than ' '. For example, python provides double leading underscore for this sort of thing.

A possibly better way to handle this is to not store any extra data on the instances being managed by the event descriptor. Instead, store references to the instances inside the event class. This is what I did in observed (see line 163).

coldfix commented 10 years ago

Whitespaces as an attribute names are pretty safe, as they can't be used accidentally by the user (a. x is just a.x), but only through explicit setattr, getattr. This is much safer than underscores which can be accessed by normal python attribute access, even double underscore names (both mangled or unmangled). If anything, I'd suggest, something like ' obsub ' to minimize the probability of another decorator/metaclass using the whitespace prefix.

Your implementation might indeed be a useful alternative, though it adds much complexity and this sort of name conflict is a rather unlikely case.

I want to throw in one additional use case, that is addressed by your implementation: it is easier for the user to add events to classes with __slots__. But, again, this is probably not a common demand (and I know of no circumstance, where it is really necessary to use).

DanielSank commented 10 years ago

I completely agree that the way observed handles this is more complex, but I do think it plays better with other code, as in your example of __slots__. However, using the strategy employed by observed would be a pretty big change. I think using ' obsub' would be an easy and useful improvement as ' ' + func.__name__ is too general.