[PATCH 4/5] Change management of Device parents to use a simple list interface.

David Lehman dlehman at redhat.com
Tue Mar 25 20:46:58 UTC 2014


On Tue, 2014-03-25 at 09:06 -0400, Anne Mulhern wrote:
> 
> 
> 
> ----- Original Message -----
> > From: "Vratislav Podzimek" <vpodzime at redhat.com>
> > To: "anaconda patch review" <anaconda-patches at lists.fedorahosted.org>
> > Sent: Tuesday, March 25, 2014 6:21:56 AM
> > Subject: Re: [PATCH 4/5] Change management of Device parents to use a simple	list interface.
> > 
> > On Mon, 2014-03-24 at 13:50 -0500, David Lehman wrote:
> > > The parent list is an instance of class ManagedList, which provides a
> > > simplified list interface with the addition of pluggable pre-add and
> > > pre-remove callbacks.
> > > 
> > > Before, to modify a device's parent set, you'd do this:
> > > 
> > >  device.parents.append(newparent)
> > >  newparent.addChild()
> > > 
> > > or this, if device is a container type:
> > > 
> > >  device._addMember(newparent)
> > > 
> > > Now, regardless of the type of device, you do this:
> > > 
> > >  device.parents.append(newparent)
> > > 
> > > Any checking or accounting is handled by the callbacks registered to
> > > the parent list.
> > > ---
> > >  blivet/devices.py     | 205
> > >  ++++++++++++++++++++++++++++++--------------------
> > >  tests/action_test.py  |   4 +-
> > >  tests/devices_test.py |  12 ++-
> > >  3 files changed, 134 insertions(+), 87 deletions(-)
> > > 
> > > diff --git a/blivet/devices.py b/blivet/devices.py
> > > index d7335c1..68230a9 100644
> > > --- a/blivet/devices.py
> > > +++ b/blivet/devices.py
> > > @@ -113,6 +113,76 @@ def deviceNameToDiskByPath(deviceName=None):
> > >          return ret
> > >      raise DeviceNotFoundError(deviceName)
> > >  
> > 
> > What about putting this class into blivet/util.py?
> > > +class ManagedList(object):
> > > +    """ A list with auditing and side-effects for additions and removals.
> > > +
> > > +        This class allows specification of callbacks to run before adding
> > > or
> > > +        removing an item. It provides a subset of the functionality
> > > provided by
> > > +        :class:`list`, making it easy to ensure the callbacks are run.
> > > +
> > > +        The following operations are implemented:
> > > +
> > > +            ml.append(x)
> > > +            ml.remove(x)
> > > +            iter(ml)
> > > +            len(ml)
> > > +            x in ml
> > > +            x = ml[i]   # not ml[i] = x
> > > +    """
> > > +    def __init__(self, items=None, appendfunc=None, removefunc=None):
> > > +        """
> > > +            :keyword items: initial contents
> > > +            :type items: any iterable
> > > +            :keyword appendfunc: a function to run before adding an item
> > > +            :type appendfunc: callable
> > > +            :keyword removefunc: a function to run before removing an item
> > > +            :type removefunc: callable
> > > +
> > > +            appendfunc and removefunc should take the item to be added or
> > > +            removed and perform any checks or other processing. The
> > > functions
> > > +            will be called immediately before adding or removing the item.
> > > +        """
> > > +        args = []
> > > +        if items:
> > > +            args.append(items)
> > > +
> > > +        self.items = list(*args)
> > I believe that these 5 lines can be replaced by simple
> > 
> >     if items:
> >         self.items = list(items)
> > 
> > because the parameter is required to be an iterable.
> > 
> > > +
> > > +        self.appendfunc = appendfunc
> > > +        self.removefunc = removefunc
> > It might be better to instantiate those callbacks to no-ops (lambda x:
> > None) instead of None if not given. That way those ifs in the
> > append/remove functions could be removed. But I'm not sure which way is
> > better.
> > 
> > > +
> > > +    def __iter__(self):
> > > +        return iter(self.items)
> > > +
> > > +    def __contains__(self, y):
> > > +        return y in self.items
> > > +
> > > +    def __getitem__(self, i):
> > > +        return self.items[i]
> > > +
> > > +    def __len__(self):
> > > +        return len(self.items)
> > > +
> > > +    def append(self, y):
> > > +        """ Add an item to the list after running a callback. """
> > > +        if y in self.items:
> > > +            raise ValueError("item is already in the list")
> > Do we really want to enforce items uniqueness here? Maybe a
> > 'unique=True' argument could be added to __init__ and checked here? Or
> > should we rename the class to make it obvious instead?
> > 
> > > +
> > > +        if self.appendfunc:
> > > +            self.appendfunc(y)
> > What about using the appendfunc's return value to determine if the item
> > should be added/removed or not? Would it be useful? Or would it just
> > cause bugs because of missing return statements and items not added?
> > 
> > > +
> > > +        self.items.append(y)
> > > +
> > > +    def remove(self, y):
> > > +        """ Remove an item from the list after running a callback. """
> > > +        if y not in self.items:
> > > +            raise ValueError("item is not in the list")
> > > +
> > > +        if self.removefunc:
> > > +            self.removefunc(y)
> > > +
> > > +        self.items.remove(y)
> > 
> > 
> > --
> > Vratislav Podzimek
> > 
> > Anaconda Rider | Red Hat, Inc. | Brno - Czech Republic
> > 
> > _______________________________________________
> > anaconda-patches mailing list
> > anaconda-patches at lists.fedorahosted.org
> > https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
> > 
> 
> I concur that ManagedList might do better in util.py.
> 
> I would prefer if the newly introduced functions in Device were more fully
> documented as to their purpose.

Good idea.

> 
> I'm puzzled by every instance where I see:
> self.parents = []
> has been introduced.
> I don't see how a ManagedList would allow that.

It doesn't. The Device.parents setter (_setParentList) removes the old
items and adds the new ones, so you get an empty ManagedList. I think
being able to initialize the list this way is quite handy.

> 
> I think that there must be a set of properties that the use of
> ManagedList for parents in devices ensures. If it
> is possible to state those explicitly, then I think it would be
> useful to have a subclass of ManagedList in devices, which is
> fully documented as to these properties
> and possibly further constrained in some way.
> I don't know what these properties might be, so I can't make
> a more concrete suggestion.

I don't want to have to pass the list itself to the add/remove
functions, so I don't think this class will be especially useful outside
of this context. Given that, I'm not sure it's worthwhile to make a
vanilla base class. If we found another use for it, we could easily
extract the base class into util.py and subclass it in the various
places that need specialized behavior.

David



More information about the anaconda-patches mailing list