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

Revision history for this message
Benji York (benji) wrote :

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. (?)

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

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?
That seems a little odd. Oh, maybe you did that because the change in
behavior broke a bunch of tests.

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.

review: Approve

« Back to merge proposal