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