Code review comment for lp:~mwaddel/linaro-image-tools/null-string-vexpress

Revision history for this message
Guilherme Salgado (salgado) wrote :

Hi Matt,

Thanks for this fix; I have just one suggestion below.

On Sat, 2011-04-02 at 05:21 +0000, Matt Waddel wrote:
[...]
> === modified file 'linaro_image_tools/media_create/boards.py'
> --- linaro_image_tools/media_create/boards.py 2011-04-01 14:18:41 +0000
> +++ linaro_image_tools/media_create/boards.py 2011-04-02 05:21:29 +0000
> @@ -287,7 +287,10 @@
> cmd_runner.run(
> ['cp', '-v', uboot_bin, boot_disk], as_root=True).wait()
>
> - boot_script_path = os.path.join(boot_disk, cls.boot_script)
> + if cls.boot_script:
> + boot_script_path = os.path.join(boot_disk, cls.boot_script)
> + else:
> + boot_script_path = ''

I think it'd make more sense to just move the line you removed to the
_make_boot_files() methods that use it. For instance, in OmapConfig
we'd have:

   boot_script_path = os.path.join(boot_dir, cls.boot_script)
   [...]
   make_boot_script(boot_env, boot_script_path)
   [...]

That means we'd duplicate the boot_script_path initialization in a few
different methods, but it's just one line and I think that's cleaner
than having it initialized in populate_boot() and passed all the way
down to _make_boot_files()

« Back to merge proposal