On Mon, Jun 20, 2011 at 04:09:09PM +0200, Ales Kozumplik wrote:
> This will avoid duplicities in the resulting kernel boot argument line.
>
> Resolves: rhbz#711002
> ---
> booty/bootloaderInfo.py | 62 +++++++++++++++++-----------------------------
> iw/zipl_gui.py | 3 +-
> language.py | 4 +-
> network.py | 31 ++++++++++-------------
> storage/devices.py | 24 +++++++++---------
> textw/zipl_text.py | 4 +-
> 6 files changed, 53 insertions(+), 75 deletions(-)
>
> diff --git a/booty/bootloaderInfo.py b/booty/bootloaderInfo.py
> index d9ab62e..8886df0 100644
> --- a/booty/bootloaderInfo.py
> +++ b/booty/bootloaderInfo.py
> @@ -87,7 +87,7 @@ def rootIsDevice(dev):
> class KernelArguments:
>
> def getDracutStorageArgs(self, devices):
> - args = []
> + args = set()
> types = {}
> for device in devices:
> for d in self.id.storage.devices:
> @@ -95,62 +95,55 @@ class KernelArguments:
> continue
>
> s = d.dracutSetupString()
> - types[s.split("=")[0]] = True
> - if s not in args:
> - args.append(s)
> + for string in s:
> + types[string.split("=")[0]] = True
> + args.update(s)

I don't like using 'string' here since it could be a module name.
Admittedly, one that shouldn't be used.

I'd also like to see dracutSetupString() renamed to something like
dracutSetupArgs() -- I was a bit confused as to what was happening since
I thought the return value was a string, not a set.

These changes are fairly extensive, so it would probably be a good idea
to cover them with some unit tests to make sure something hasn't been
missed.

Other than that it looks pretty good, much cleaner than using strings.

--
Brian C. Lane | Anaconda Team | IRC: bcl #anaconda | Port Orchard, WA (PST8PDT)
_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list

On 06/20/2011 07:11 PM, Brian C. Lane wrote:

I don't like using 'string' here since it could be a module name.
Admittedly, one that shouldn't be used.


I renamed it to 'setup_arg'.


I'd also like to see dracutSetupString() renamed to something like
dracutSetupArgs() -- I was a bit confused as to what was happening since
I thought the return value was a string, not a set.


Right, renamed to dracutSetupArgs everywhere.


These changes are fairly extensive, so it would probably be a good idea
to cover them with some unit tests to make sure something hasn't been
missed.


I thought about that but it is split over several modules and needs a
lot of different objects to be instantiated and none of them are under
the test harness yet. Perhaps on master once the bootloader has unit tests.


Ales

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list

On Tue, Jun 21, 2011 at 10:34:12AM +0200, Ales Kozumplik wrote:
> On 06/20/2011 07:11 PM, Brian C. Lane wrote:
> >I don't like using 'string' here since it could be a module name.
> >Admittedly, one that shouldn't be used.
>
> I renamed it to 'setup_arg'.
>
> >I'd also like to see dracutSetupString() renamed to something like
> >dracutSetupArgs() -- I was a bit confused as to what was happening since
> >I thought the return value was a string, not a set.
>
> Right, renamed to dracutSetupArgs everywhere.
>
> >These changes are fairly extensive, so it would probably be a good idea
> >to cover them with some unit tests to make sure something hasn't been
> >missed.
>
> I thought about that but it is split over several modules and needs
> a lot of different objects to be instantiated and none of them are
> under the test harness yet. Perhaps on master once the bootloader
> has unit tests.
>

Sounds reasonable. Ack

--
Brian C. Lane | Anaconda Team | IRC: bcl #anaconda | Port Orchard, WA (PST8PDT)
_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list