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

Hi Antonio,

On Thu, 2013-04-04 at 00:22 +0000, Antonio Terceiro wrote:
> > # 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.

I agree that None is the correct default, which I fixed in the code. But
for devices, os, environment having an empty string as default while
retrieving the values from the dict is fine, since we do a join of these
strings with a comma.

> > === 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)?

That is what emacs python mode gives me. I tried a tab on that line, but
emacs refused to align it! Hope this the correct indentation? (I go by
emacs ;) )

I fixed the comments which you mentioned above, please go through it and let me know
if it is good to be merged.

Thank You.

--
Senthil Kumaran
http://www.stylesen.org/
http://www.sasenthilkumaran.com/