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

Vratislav Podzimek vpodzime at redhat.com
Tue Mar 25 20:20:35 UTC 2014


On Tue, 2014-03-25 at 09:47 -0500, David Lehman wrote:
> On Tue, 2014-03-25 at 11:21 +0100, Vratislav Podzimek wrote:
> > 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?
> 
> I am definitely open to putting it somewhere other than devices.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.
> 
> I've updated the docstring in my local tree:
> 
>  appendfunc and removefunc should take the item to be added or
>  removed and perform any checks or other processing. The callbacks
>  will be run immediately before adding or removing the item. The
>  :class:`~.ManagedList` instance is not passed to the callback. While
>  this is not optimal for general-purpose use, it is ideal for the
>  intended use as part of :class:`~.Device`
Looks good to me.

> 
> But I also need to add something about the set-like uniqueness
> constraint.
> 
> > > +        """
> > > +        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.
> 
> It is also optional, and list(None) raises a TypeError.
That's why I've kept the 'if items' check.

> 
> > 
> > > +
> > > +        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.
> 
> Either way sounds about the same to me.
> 
> > 
> > > +
> > > +    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?
> 
> Between the uniqueness requirement and the fact that the callback
> doesn't pass the list instance, I think I should rename the class to
> ParentList since it is quite specialized. It's possible to enforce the
> uniqueness in Device._addParent instead, and to pass the list instance
> to the callback, but both are a bit inconvenient.
Agreed, I think renaming the class to ParentList and changing docstrings
should be okay. If it turns out we want such functionality also for
something else and with different constraints, we can always refactor
the common code out to a separate class that could be inherited by the
others.

> 
> > 
> > > +
> > > +        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?
> 
> The approach I've taken is to raise an exception in the callback if you
> don't want the item added. I suppose that could be a bit drastic. As
> long as we document the importance of callback return value we should be
> able to make use of that as well.
I believe we will be able to make sure return values are correct and
this could easily make the callbacks "stronger".

-- 
Vratislav Podzimek

Anaconda Rider | Red Hat, Inc. | Brno - Czech Republic



More information about the anaconda-patches mailing list