[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