[MERGE] Use slots consistently in InventoryEntry hierarchy -- update

[Jan Hudec]

On Thu, Jun 08, 2006 at 02:51:21 +1000, Robert Collins wrote:
> On Wed, 2006-06-07 at 14:43 +0100, Martin Pool wrote:
> > On  1 Jun 2006, Jan Hudec <bulb at ucw.cz> wrote:
> > 
> > > Ok. I have tried to update the patch to address this and Aaron's concerns.
> > > The 'kind' is now a normal class attribute and so are all the other
> > > attributes specific to some InventoryEntry subclass. So the serializers can
> > > safely read all of them and get None for the unapplicable ones, so they are
> > > kind-agnostic again (I actually reverted them to the old code except
> > > replacing the 'something != None' constructs with proer 'something is not
> > > None'.
> > > 
> > > Please review. The original version got +1, -0, +1. I am attaching both the
> > > complete diff from mainline (ok, 3 revisions back at this point) and a diff
> > > of just the updates. Please use
> > > http://drak.ucw.cz/~bulb/bzr/bzr/inventory-slots for merging.
> > 
> > +1, ok with me.  Thanks.
> 
> This has two current problems:
>  * Tests fail - showstopper.

Hm, I see that a conflicting (as in expecting attributes on inventory entries
of kind for which they don't make sense) change got in between I generated
the patch and it was approved. So I'll update it.

>  * The class attributes on InventoryEntry are IMNSHO quite ugly.
> 
> I'd really love to see:
> InventoryEntry has slots for name, parent_id, file_id
> InventoryFile has those slots and slots for text_sha and size
> InventoryLink has the I.E. slots and symlink_target
> InventoryDirectory has children.
> 
> There should be no attribute on class or instance for e.g.
> symlink_target except on InventoryLink.

That's what the previous version of the patch did. And I got complaint that
it shouldn't be checking for the kind. So I tried this, because
benchmarking says it's faster than checking with hasattr or try/catch.

> This *may* increase the amount of current 'if kind ==' tests in the
> codebase, but I think that is an acceptable *interim* tradeoff. Each one
> of those should have a #FIXME attached to it to use an appropriate means
> of instance based dispatch - either a method on InventoryEntry, or
> double-dispatch via a visit() style method.

Well, it's mainly just the serializer and that kind of needs to know a bit
about the inventory entry. I think visitors would be kind of overkill, but
a list of attributes (a method to return a list of attributes that need
serializing) would be appropriate for this.

> I realise this patch is getting ridiculous in terms of thrashing, so I
> ask that Martin/John/Aaron agree on this goal I'm setting so we dont
> waste Jan's effort.
> 
> -1 (sorry) as it stands.
> Rob


-- 
						 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/20060607/3dadbe20/attachment.pgp