[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