[rfc] An object that creates (nested) attributes automatically on assignment

[Steven D'Aprano]

On Fri, 10 Apr 2009 19:04:38 -0700, Edd wrote:

> Hi folks,
> 
> I'd like to use Python itself as the configuration language for my
> Python application. I'd like the user to be able to write something like
> this in their config file(s):
> 
>    cfg.laser.on = True
>    cfg.laser.colour = 'blue'
>    cfg.discombobulated.vegetables = ['carrots', 'broccoli'] # ...
> 
> To this end, I've created a class that appears to allow instance
> variables to be created on the fly.

Um, don't all classes allow that?

>>> class MyClass(): pass
...
>>> instance = MyClass()
>>>

Or do you mean instance *attributes*? Again, apart from built-in types 
and classes that use __slots__, all classes allow that.

>>> instance.parrot = 'Norwegian Blue'
>>>

> In other words, I can to the
> following to read a config file:
> 
>     cfg = Config()
>     execfile(filename, {'cfg', cfg}, {})


That's okay so long as you trust the user not to put malicious, or buggy, 
code in your config file. Personally, I think config files should be more 
tolerant of errors than a programming language.


> However, I think my implementation of the Config class is a little
> crappy. I'd really appreciate the critical eye of a pro. Here's the
> sauce:

For starters, where is your documentation? No doc strings, not even any 
comments! No, I tell a lie... *one* obscure comment that doesn't really 
explain much.

 
>     class Config(object):
>         def __init__(self, sealed=False):
>             def seal():
>                 for v in self._attribs.values():
>                     if isinstance(v, self.__class__): v.seal()
>                 del self.__dict__['seal']
> 
>             d =  {'_attribs': {}, '_a2p': None}
>             if not sealed: d['seal'] = seal
> 
>             self.__dict__.update(d)

I'm going to try to guess what the above does. When you initialise an 
instance, you can tell the instance to be "sealed" or unsealed. I'm not 
sure what the difference is, or why you would choose one over the other. 
Sealed instances seem to be exactly the same as unsealed instances, 
except they have a seal() method (actually a closure). The seal method, 
when called, recursively seals any embedded Config instances inside the 
current instance, then deletes itself.

Arghhh!!! Self-modifying code!!! Unclean, unclean!!!

I'm not sure why seal() is necessary -- it seems to me that if present, 
all it does is delete itself. So why not just leave it out altogether?

You also have a rather complicated way of adding instance attributes. 
Instead of 

d =  {'_attribs': {}, '_a2p': None}
self.__dict__.update(d)

why not just do the more obvious:

self._attribs = {}
self._a2p = None

?

 

>         def __getattr__(self, key):
>             if not key in self._attribs:
>                 d = Config(sealed='seal' not in self.__dict__) def
>                 add2parent():
>                     self._attribs[key] = d
>                     if self._a2p:
>                         self._a2p()
>                         self._a2p = None


It looks like you are just re-inventing the normal attribute mechanism of 
Python. I'm not sure why you feel this is necessary. And it contains MORE 
self-modifying code! Yuck! Frankly I don't care enough to dig into your 
code to understand how it works in detail.



>                 # if anything is assigned to an attribute of d, # make
>                 sure that d is recorded as an attribute of this
> Config
>                 d._a2p = add2parent
>                 return d
>             else:
>                 return self._attribs[key]
> 
>         def __setattr__(self, key, value):
>             if key in self.__dict__:
>                 self.__dict__[key] = value
>             else:
>                 if not 'seal' in self.__dict__:
>                     clsname = self.__class__.__name__
>                     raise AttributeError("'%s' object attribute '%s'
> is read-only (object is sealed)" % (clsname, key))
>                 self.__dict__['_attribs'][key] = value if self._a2p:
>                     self._a2p()
>                     self._a2p = None

Does "sealed" mean that the instance is read-only? If so, and if I'm 
reading this correctly, I think it is buggy. You allow modifications to 
attributes inside __dict__ *without* checking to see if the instance is 
read-only. Then you get the test backwards: surely the existence, not the 
absence, of a 'seal' attribute should mean it is sealed?



>         def __delattr__(self, key):
>             if key in self.__dict__:
>                 clsname = self.__class__.__name__
>                 raise AttributeError("can't delete '%s' object
> attribute '%s' as it is used for book-keeping!" % (clsname, key))
>             else:
>                 if key in self._attribs:
>                     del self._attribs[key]

Nothing much to say here, except that you're doing more work re-inventing 
the wheel, storing attributes inside _attribs instead of using the 
general attribute mechanism. Seems unnecessary to me, but perhaps I don't 
understand your use-case.

 
> Once the config is loaded, it will be passed down to other user- written
> scripts and it's important that these scripts don't accidentally change
> the config. So the idea is that I'll call cfg.seal () to prevent any
> further changes before passing it on to these other scripts.

Or you could pass a *copy* of the config, and let them change it to their 
heart's content, it won't matter.

Or you could say "we're all adults here", simply document that any 
changes will have consequences, and let user scripts change the config. 
And why not?


> Beyond the
> general fiddliness of the code, I think the way seal() currently works
> is particularly pants.

Is "pants" slang for "fragile, hard to understand and difficult to debug"?



-- 
Steven