On Tue, Aug 02, 2011 at 06:37:22PM -0700, Brock Pytlik wrote:
> On 07/22/11 14:25, Brock Pytlik wrote:
> >Webrev:
> >https://cr.opensolaris.org/action/browse/pkg/bpytlik/18439-v1
> >
> New webrev:
> https://cr.opensolaris.org/action/browse/pkg/bpytlik/18439-v2
>
sorry for the delay.
i still need to review the tests.
comments on the code are below.
ed
----------
general
- i thought that the last time we talked about this we had agreed on
putting a copy of the "parsable_version" into the progress tracker
class? if we did this we wouldn't have to add an extra parameter to
all the linked image functions, and we wouldn't have to scan the
arguments in __pkg_cmd().
----------
src/modules/client/api.py
- in __plan_op() this comment seems wrong:
Eventually these will yield plan descriptions objects as well.
i thought eventually we'd yield plan descriptions objects instead.
- in __plan_op() we should not plan all children and then return the
results. instead do_recurse() should yield results as they become
available.
- all the gen_plan_*() functions have the following comments, which are
now wrong:
This is a generator function that returns PlanDescription objects
- i don't think we should bother with parsable output mode for
detach_linked_children() since that will never change any packages.
(it shouldn't even be going through the solver, see bug 18552.)
- the detach_linked_children() docstring describes the tuple returned by
linked image operations. please update this for the new value your
adding.
- instead of adding the None p_dict value to the return code in
audit_linked() could you add it in audit_self()? (just for
consistency so that all the LI interfaces return the same tuple.)
----------
src/modules/client/imageplan.py
- should mediators() be __mediators()? (is it accessed outside of the
class?)
----------
src/modules/client/linkedimage/common.py
- in __rvdict2rv(), when you merge all the p_dict values into a p_dicts
list why not drop any None values?
- iirc you said that if there were any error you don't want to return
any parsable results. if that's the case thein in __rvdict2rv()
perhaps we should do:
err = apx.LinkedImageException(bundle=errs)
p_dicts = [] <--- add this line
- i'm not sure how the python subrpocess module deals with pipes and
signals. have you tested what happens if you kill a child process
while the parent attempting to read from the stdout pipe? (this can
generate a signal in the parent and unless python catches it and
handles it you might want to use a temporary file instead of pipe.)
----------
src/client.py
- why is __display_parsable_plan() part of the cli client? shouldn't it
be part of the PlanDescription class itself? having the
PlanDescription output format be part of the client seems weird.
other API consumers might want to output this format, etc. also,
presumably we might eventually want to have the plan description
reload a parsable plan so a python consumer could manipulate it
programatically?
- in __display_parsable_plan(), why pass in an api_inst instead of just
passing in the plan for the current image?
- in __display_parsable_plan(), this seems weird:
space_available = child_images[0]["space-available"]
if there's no plan for the parent, why not just default this to 0 vs
what is essentially a random value?
- in display_plan(), if we're in parsable output mode we don't display
any licenses or mark them as installed. is this correct? if so it
seems like parsable output mode can't actually be used to install
packages? if so, perhaps -n should be required for parsable output
mode?
- in __api_op(), please add "parsable_version" to the kwargs array at
the same place where we add "noexecute" and other options.
- in __api_op(), in the first call to display_plan, child_plans is
always an empty list, so why not just specify [] (or assert that it's
an empty list).
- you duplicate the "try: __display_parsable_plan(...) except ..." code
block in lots of places. how about a function?
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
