[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