[MERGE] Use slots consistently in InventoryEntry hierarchy -- regenerated against new head. Seeking another reviewer.

[Jan Hudec]

On Tue, May 30, 2006 at 19:23:44 -0400, Aaron Bentley wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> I'm -0 on this patch.  I had previously not reviewed it because I don't
> understand slots well enough to critique them.  But there are two parts
> here:
> 
> 1. handle slots differently
> 2. remove attributes from InventoryEntry and its subclasses.
> 
> I'm not in favour of 2, because InventoryEntries are more like structs
> than classes.  They're blobs of data without much behaviour.  This means
> that the behaviour is supplied by external code.
> 
> Previously, InventoryDirectories had a text_sha1 of None.  Now they have
> no such property, which means that code that wants to examine the
> text_sha1 must first ensure that it's not looking at a directory.
> 
> This code is now 50% longer than it was before, and as a result of these
> changes, new code will be longer, and more type-sensitive, too.
> 
> If we're going to start customizing attributes of InventoryEntries, then
> we should also provide more behaviour, so that we don't become more
> type-sensitive.  For example, the serializer should not need to know
> which attributes are available on each kind of InventoryEntry.  Some
> options:
> a) Each InventoryEntry could provide a get_attribute_list() method,
> which would provide a list of name,value tuples.
> 
> b) Each InventoryEntry could provide a set_attributes() method, which
> would take as an argument an object that it can store its attributes in

These two are a good idea. Thanks.

I want to do the properties stuff. I originally wanted to convert the
base attributes to slots (with the patch I sent) and add a proplist
dictionary with the other ones (executable + properties defined by
plugins (eg. newline-style, keyword expansion, charset conversion, unix
permissions etc.). Now that would add a dictionary back, right? And
quite many operations would have to look at it anyway. So this leads me
to thinking that perhaps I should unify the core and non-core attributes
and use methods like you suggested for everything, not just the
proplist.

> c) Each InventoryEntry could provide all attributes, but irrelevent ones
> would be properties that always returned None.  e.g.:
> class InventoryDirectory(InventoryEntry)
> ~    text_sha1 = property(lambda self: None)
> 
> d) Instead of assuming attribute access will always succeed, the
> serializer could handle missing attributes gracefully:
> ~    if getattr(ie, 'text_size', None) is not None:
> ~        e.set('text_size', ie.text_size

-- 
						 Jan 'Bulb' Hudec <bulb at ucw.cz>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060531/770f130e/attachment.pgp