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

Anne Mulhern amulhern at redhat.com
Tue Mar 25 13:06:09 UTC 2014





----- 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.

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

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.

- mulhern








More information about the anaconda-patches mailing list