[PATCH 4/5] Use generator instead of list for auxiliary old_disk_names variable
Anne Mulhern
amulhern at redhat.com
Fri Apr 4 16:09:59 UTC 2014
----- Original Message -----
> From: "Vratislav Podzimek" <vpodzime at redhat.com>
> To: "anaconda patch review" <anaconda-patches at lists.fedorahosted.org>
> Sent: Friday, April 4, 2014 11:12:52 AM
> Subject: Re: [PATCH 4/5] Use generator instead of list for auxiliary old_disk_names variable
>
> On Fri, 2014-04-04 at 08:57 -0400, Anne Mulhern wrote:
> >
> >
> >
> > ----- Original Message -----
> > > From: "Vratislav Podzimek" <vpodzime at redhat.com>
> > > To: anaconda-patches at lists.fedorahosted.org
> > > Sent: Thursday, April 3, 2014 7:56:36 AM
> > > Subject: [PATCH 4/5] Use generator instead of list for auxiliary
> > > old_disk_names variable
> > >
> > > Signed-off-by: Vratislav Podzimek <vpodzime at redhat.com>
> > > ---
> > > pyanaconda/ui/gui/spokes/custom.py | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/pyanaconda/ui/gui/spokes/custom.py
> > > b/pyanaconda/ui/gui/spokes/custom.py
> > > index 5914d4c..f5426f0 100644
> > > --- a/pyanaconda/ui/gui/spokes/custom.py
> > > +++ b/pyanaconda/ui/gui/spokes/custom.py
> > > @@ -906,7 +906,7 @@ class CustomPartitioningSpoke(NormalSpoke,
> > > StorageChecker):
> > > # the disks need to be updated since we've
> > > replaced
> > > all
> > > # of the devices with copies in the
> > > devicefactory
> > > error
> > > # handler
> > > - old_disk_names = [d.name for d in old_disks]
> > > + old_disk_names = (d.name for d in old_disks)
> > > old_disks =
> > > [self._storage_playground.devicetree.getDeviceByName(n)
> > > for n in old_disk_names]
> > > try:
> > > self._replace_device(old_device_type,
> > > device.size,
> > > --
> > > 1.9.0
> > >
> > > _______________________________________________
> > > anaconda-patches mailing list
> > > anaconda-patches at lists.fedorahosted.org
> > > https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
> > >
> >
> > I'm dubious about this because save_right_side is such a huge method, and
> > there's
> > a risk that someone will reuse old_disk_names later in the method,
> > which will be empty if it is a generator.
> Well, it's hard to say. I like using generators like that instead of
> having complex and long list comprehensions. One can nicely apply
> various filters and changes in a series of generators and then create
> the list in a short list comprehension in the end.
>
> >
> > I would either
> > substititute that generator expression for the use of old_disk_names, and
> > get rid of old_disk_names completely OR
> > leave it alone.
> >
> > Ideally save_right_side can be made smaller, right now it's 1134 - 621
> > lines (which is too big for me to find my way easily).
> > Note the indentation here, character 25. That's kind of deeply nested.
> Yeah, it's a crazy method. That's why I refactored out the mountpoint
> validation code and I'm now finishing some more patches, this time
> refactoring out the code for replacing device etc.
>
> --
> 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
>
Well, keep up the good work :)
If this generator were in a smaller method it wouldn't have bothered me a bit.
- mulhern
More information about the anaconda-patches
mailing list