[MERGE] Use slots consistently in InventoryEntry hierarchy -- regenerated against new head. Seeking another reviewer.
aaron.bentley at utoronto.ca
Wed May 31 00:23:44 BST 2006
-----BEGIN PGP SIGNED MESSAGE-----
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
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
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
c) Each InventoryEntry could provide all attributes, but irrelevent ones
would be properties that always returned None. e.g.:
~ 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)
Also, "foo != None" is bad form. I realise there were some examples of
that in the serializer, but it's usually better to do "foo is not None"
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org
-----END PGP SIGNATURE-----
More information about the bazaar