Code review comment for lp:~bac/charmworld/bundles-display

> The branch looks good. Here are some semi-bikesheddy comments.
>
> It's kind of strange that construct_basket_id returns two values while
> all the other "construct_*_id" functions just return the ID. Perhaps
> two functions would be better. (?)

I renamed to set_basket_info and modify the basket_data dictionary in place.

>
> Does "make lint" complain about the new unused variables?

Nope. Which ones?

>
>
> Re. get_bundle_data: so now if you don't specify an owner argument you
> get an empty string, but if you pass owner=None you get a random string?

No, both cases give you a random string.

> That seems a little odd. Oh, maybe you did that because the change in
> behavior broke a bunch of tests.

No, any broken tests were fixed.

>
> Another thing, about get_bundle_data: I always thought it was weird to
> use the local namespace as a sandbox in which to construct the return
> value. Now that you're doing more in the function and having to do the
> "del rep[k]" at the end it's getting more strained. You might consider
> revamping the function to use **charm_data as the parameter list instead
> and then manipulating charm_data instead of the locals.

I've fixed it to be more explicit.

Thanks for the review and good suggestions.