[MERGE] Use slots consistently in InventoryEntry hierarchy

[Jan Hudec]

On Thu, May 11, 2006 at 10:59:30 -0700, John A Meinel wrote:
> Jan Hudec wrote:
> > Hello All,
> > 
> > Earlier I complained about inconsistent usage of __slots__ in InventoryEntry
> > (http://thread.gmane.org/gmane.comp.version-control.bazaar-ng.general/11220/)
> > I have given it a look today and created a patch that adds __slots__ in all
> > InventoryEntry subclasses. I have also moved the attributes to the classes
> > for which they actually have a meaning (eg. no children in file). I have
> > fixed the code so that all tests pass.
> > 
> > The change can be merged from http://drak.ucw.cz/~bulb/bzr/bzr/inventory-slots
> > Attaching a diff for review.
> > 
> > Regards,
> > 
> > Jan
> > 
> > 
> > 
> > ------------------------------------------------------------------------
> > 
> > === modified file 'bzrlib/bzrdir.py'
> > --- bzrlib/bzrdir.py	
> > +++ bzrlib/bzrdir.py	
> > @@ -1575,7 +1575,8 @@
> >                  # if this fails, its a ghost ?
> >                  assert old_revision in self.converted_revs 
> >          self.snapshot_ie(previous_entries, ie, w, rev_id)
> > -        del ie.text_id
> > +        if ie.kind == 'file':
> > +            del ie.text_id
> >          assert getattr(ie, 'revision', None) is not None
> >  
> >      def snapshot_ie(self, previous_revisions, ie, w, rev_id):
> > 
> 
> We haven't used 'text_id' in quite a while. Are there places that
> actually use it? Otherwise, I would be in favor of just ignoring it.

Yes, there is one. We still support upgrading from version 4 and that needs
it.

> > === modified file 'bzrlib/inventory.py'
> > --- bzrlib/inventory.py	
> > +++ bzrlib/inventory.py	
> > [...]
> > @@ -503,15 +492,18 @@
> >          return (self.file_id == other.file_id) \
> >                 and (self.children == other.children)
> >  
> > +    def sorted_children(self):
> > +        l = self.children.items()
> > +        l.sort()
> > +        return l
> 
> You need to update the __eq__ entry, so that it will check 'children'
> when it is checking the rest of the stuff. Just like you did for
> InventoryDirectory.

It was already there, so I did not touch it!

It says:

    def __eq__(self, other):
        if not isinstance(other, RootEntry):
            return NotImplemented

        return (self.file_id == other.file_id) \
               and (self.children == other.children)

Note, that it does not compare revision, so it's probably wrong, but that's
how it was. Do you think I should change it?

> I could also see making Root a child of Directory, but I'm not settled
> on that.

Yes, I thought about that too. But though logically the inheritance should be
InventoryDirectory(RootEntry), functionally it's not---RootEntry can do a lot
less and some things a bit differently. So I'll keep them independent for the
time being.

> > @@ -625,6 +628,10 @@
> >      def __init__(self, file_id, name, parent_id):
> >          super(InventoryFile, self).__init__(file_id, name, parent_id)
> >          self.kind = 'file'
> > +        self.executable = False
> > +        self.text_sha1 = None
> > +        self.text_size = None
> > +        self.text_id = None
> 
> Again, no need for text_id. Unless we have code that supports upgrading
> the old old branch format into later formats. But since we haven't
> supported writing the format for a long time, I would still argue that
> we should strip out any support for it, now that we have had enough time
> to deprecate it.

We do have the code. It's even excercised by the test suite. Maybe it could
be removed now, but I'd prefer doing one thing at a time.

> > === modified file 'bzrlib/tests/test_inv.py'
> > --- bzrlib/tests/test_inv.py	
> > +++ bzrlib/tests/test_inv.py	
> > @@ -84,12 +84,12 @@
> >  
> >      def test_dir_detect_changes(self):
> >          left = inventory.InventoryDirectory('123', 'hello.c', ROOT_ID)
> > -        left.text_sha1 = 123
> > -        left.executable = True
> > -        left.symlink_target='foo'
> > +        #left.text_sha1 = 123
> > +        #left.executable = True
> > +        #left.symlink_target='foo'

... I should have probably removed the comments or turned them into
assertRaises.

> > === modified file 'bzrlib/xml5.py'
> > --- bzrlib/xml5.py	
> > +++ bzrlib/xml5.py	
> > @@ -14,7 +14,7 @@
> >  
> >  
> >  from bzrlib.xml_serializer import ElementTree, SubElement, Element, Serializer
> > -from bzrlib.inventory import ROOT_ID, Inventory, InventoryEntry
> > +from bzrlib.inventory import ROOT_ID, Inventory, InventoryEntry, InventoryFile
> >  import bzrlib.inventory as inventory
> >  from bzrlib.revision import Revision        
> >  from bzrlib.errors import BzrError
> > @@ -50,16 +50,23 @@
> >          e.set('name', ie.name)
> >          e.set('file_id', ie.file_id)
> >  
> > -        if ie.text_size != None:
> > -            e.set('text_size', '%d' % ie.text_size)
> > -
> > -        for f in ['text_sha1', 'revision', 'symlink_target']:
> > -            v = getattr(ie, f)
> > -            if v != None:
> > -                e.set(f, v)
> > -
> > -        if ie.executable:
> > -            e.set('executable', 'yes')
> > +        if ie.kind == 'file':
> > +            assert isinstance(ie, InventoryFile)
> 
> I think we try to avoid assert isinstance, preferring to use duck-typing
> as much as possible.
> 
> You could have also just changed the line to:
> 
> v = getattr(ie, f, None)
> 
> Since that will return None when the attribute doesn't exist.

I will get rid of the assert. But for the rest I prefer being explicit about
which type should have which attributes.

> > [...] 
> I like the work, just needs some refinements (or at least discussion).

Ok, I'll fix the assert and add the assertRaises checks.

-- 
						 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/20060511/6455562f/attachment.pgp