Hi Michael, I'm CC'ing the Storm list, since these kinds of conversations are nice to have in the open.
On Tue, Aug 23, 2011 at 12:35 PM, Michael Löffler <[email protected]> wrote: > as you seem to be involved in active developement of storm, and as it's just > a minor thing, I'd like to suggest one change directly to you. I saw the > initialisation of Property: > > class Property(object): > > def __init__(self, name=None, primary=False, > variable_class=Variable, variable_kwargs={}): > self._name = name > > In my eyes, using {} in the function header here is quite dangerous, as it > can lead to side effects, which I think in the case of the Property object > are not really expected and wished: > > In [1]: class Test(object): > ...: def __init__(self, data={}): > ...: self.data = data > > In [2]: a = Test() > > In [3]: a.data > Out[3]: {} > > In [4]: a.data[3] = 5 > > In [5]: c = Test() > > In [6]: c.data > Out[6]: {3: 5} > > In this special case, it looks like most of the time, the empty dictionary > should not be touched afterwards. But in case it happens, it would also > change properties in places, where it would not be expected. In this particular case variable_kwargs is eventually expanded when it's used to instantiate a PropertyColumn, so there is no real safety issue here. Also, Property._variable_kwargs is a private attribute, so no one should be messing with it without knowing what they're doing. Anyway, I tend to agree that defining an empty dict in a keyword argument isn't so great, but I guess it was a micro-optimization to avoid creating a new empty dict for every Property that gets instantiated. Thanks, J. -- storm mailing list [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/storm
