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

Vratislav Podzimek vpodzime at redhat.com
Tue Mar 25 10:21:56 UTC 2014


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



More information about the anaconda-patches mailing list