Edward Pilatowicz wrote:
i've updated the webrev with your feedback.
my replies are inline below.
ed
On Thu, Jul 16, 2009 at 05:06:49PM -0700, Brock Pytlik wrote:
Edward Pilatowicz wrote:
hey all,
it's taken me longer than i hoped, but i finally have a webrev ready
that implements change variant functionality. you can check it out
here:
http://cr.opensolaris.org/~edp/pkg-vc/
this is my first shot at ips and python development, so i've probably
done a lot of bone headed things in this wad. please feel free to point
them out. i've also carried over a lot of ON and C 'isms with me, so
please feel free to try and beat them out of me (i promise not to
protest TOO much. ;).
thanks
ed
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
General comment:
I really like how you cleaned up and modularized the code in client.py
and api.py. Thanks for doing that. ...
cool. i didn't know how those changes would be recieved.
... In the same spirit, (client.py) lines
619-650 could probably be extracted and shared between different
planning functions as well since they encounter a similar set of
exceptions. We could pass in the function of the object we're going to
call and the list of arguments. Whether or not we should do that is a
different issue, and I know my ideas on code elegance aren't always
sane, but it might centralize our code even further.
well, unfortunatly the planning functions are all pretty different wrt
args and return values. what is pretty similar between them is the
exception handling. so what i could do is move all the exception
handlers into one function that could be invoked whenever a planning
function induced an exception (similar to what i did with
__plan_common_exception()).
if this sound good to you let me know and i'll do it.
Well, let me show you roughly what I had in mind:
__plan_func(func, *args, **kwargs):
try:
ret = func(*args, **kwargs)
return True (or success, or whatever that means no exception
happened), ret
except <and here add in all the exception handlers>
then in install (for example):
success, ret_val = __plan_func(api_inst.plan_install, pkg_list, filters,
refresh_catalogs, noexecute, verbose=verbose,
update_index=update_index)
if not success:
return 1 (or success, or whatever)
stuff_to_do, cre = ret_val
... continue on with code.
Image update planning bit would become:
success, ret_val = __plan_func(api_inst.plan_update_all, sys.argv[0],
refresh_catalogs,
noexecute, force=force, verbose=verbose,
update_index=update_index, be_name=be_name)
if not success:
return 1 (or success, or whatever)
stuff_to_do, opensolaris_image, cre = ret_val
... continue on with code.
(Sorry if that got mangled by email, if it did, let me know and I'll put
a webrev together or paste it somewhere.)
Now, what I'm not sure is whether making that change is a good/cool use
of python, or a cringe inducing nightmare ;) It should work though
(modulo typos).
Almost everything else here is just style nits.
client.py:
629: I think we've taken the approach of not exposing the name of the
exception to the user if it's an expected exception, and letting it
handle whether or not to identify its class.
ok. well, we still expose the exception type for inventory failures
with pkg image update, install, and uninstall. i've gone ahead and
updated all of these cases (change variant, image update, install, and
uninstall) to NOT display the exception name.
my fault, I thought you had introduced that (inventory exception) thing.
Since it's what we have now, I don't feel strongly about removing it.
pkgplan.py:
85: I'm not crazy about this approach, but I can live with it. I guess
personally, I'd rather see propose_reinstall take two fmris as
arguments, and assign those to origin and destination fmri, but that
could well just be my own weird preferences.
hm. but if your doing a re-install of an fmri, in what case would the
origin and destination fmri differ? if your reinstalling a package
wouldn't the origin and destination always have to be the same?
(otherwise it wouldn't be a reinstall, right?)
Right, I think my objection is to calling the init function, then
immediately changing the value of the origin fmri at line 85. That just
feels... wrong? to me. I wish I could articulate it better, but if I'm
the only one who it rubbed the wrong way, don't worry about me.
baseline.txt:
think we standardized on alphabetical order here
well, if you have that's weird because sorting the file produces
all sorts of extra diffs. ;)
that said, i think that keeping it sorted is a good idea so why not
start now. ;)
Either way is fine with me. I thought we had been keeping it sorted (by
hand), but obviously I'm wrong there too :D
man page changes? or is this not for public consumption yet?
hm. i didn't realize that the pkg man page is in the pkg gate. go
figure. ;) yes, i can document this and i see no reason to hide this
feature. i'll update the man page and send out another updated webrev
when i do.
That's what I figured :)
thanks for looking at this.
My pleasure.
ed
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
