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

On Fri, Apr 05, 2013 at 09:33:20AM -0000, Senthil Kumaran S wrote:
> 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.

Cool.

I like get('foo') -- how it was before -- more then get('foo', None), though.

> 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.

The joins are already inside if's that test for the presence of those
properties. If present, are't those properties guaranteed to be arrays
according to the schema?

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