Code review comment for lp:~stylesen/lava-dispatcher/evolve-bundle-stream-1.6

Hi Senthil,

In general, I like the fact that we are getting the diff smaller and
smaller. :-)

Follows another round of comments, which is basically one issue really:
the issue of default values - I think it should be `None`, and would
like your opinion.

 review needs-info

> === modified file 'lava_dispatcher/actions/lava_test_shell.py'
> --- lava_dispatcher/actions/lava_test_shell.py 2013-02-11 02:31:22 +0000
> +++ lava_dispatcher/actions/lava_test_shell.py 2013-04-02 01:15:27 +0000
> @@ -204,20 +204,20 @@
>
> def _get_testdef_info(testdef):
> metadata = {'os': '', 'devices': '', 'environment': ''}
> - metadata['version'] = str(testdef['metadata'].get('version'))
> - metadata['description'] = str(testdef['metadata'].get('description'))
> - metadata['format'] = str(testdef['metadata'].get('format'))
> + metadata['description'] = testdef['metadata'].get('description', '')
> + metadata['format'] = testdef['metadata'].get('format', '')
> + metadata['version'] = testdef['metadata'].get('version', '')
>
> # Convert list to comma separated string.
> if testdef['metadata'].get('os'):
> - metadata['os'] = ','.join(testdef['metadata'].get('os'))
> + metadata['os'] = ','.join(testdef['metadata'].get('os', ''))
>
> if testdef['metadata'].get('devices'):
> - metadata['devices'] = ','.join(testdef['metadata'].get('devices'))
> + metadata['devices'] = ','.join(testdef['metadata'].get('devices', ''))
>
> if testdef['metadata'].get('environment'):
> metadata['environment'] = ','.join(
> - testdef['metadata'].get('environment'))
> + testdef['metadata'].get('environment', ''))

So you changed from using `"None"` to using an empty string as the
default. Why are we doing this? Is this the expected behavior?

If you agree with me that None is indeed the appropriate default here, I
think you should just undo the diff lines above.

> === modified file 'lava_dispatcher/lava_test_shell.py'
> --- lava_dispatcher/lava_test_shell.py 2013-02-11 02:31:22 +0000
> +++ lava_dispatcher/lava_test_shell.py 2013-04-02 01:15:27 +0000
> @@ -284,6 +284,25 @@
> return attachments
>
>
> +def _get_run_testdef_metadata(test_run_dir):
> + testdef_metadata = {
> + 'version': '',
> + 'description': '',
> + 'format': '',
> + 'location': '',
> + 'url': '',
> + 'os': '',
> + 'devices': '',
> + 'environment': ''
> + }

minor nitpick: shouldn't this closing brace align with testdef_metadata (i.e. one
indent level to the left)?

Main point here is: why are you using an empty string as default values,
instead of `None`? Isn't `None` the appropriate value in case no data
was provided? Shouldn't we s/''/None/ in the above hunk?

--
Antonio Terceiro
Software Engineer - Linaro
http://www.linaro.org