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. > Almost everything else here is just style nits. > > client.py: > 606: might be nice to tell the user what variant was specified multiple > times (in the vague future where a user is changing 42 variants) > done > 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. > 639: No need to wrap e in _() > there were a bunch of instances of this. i've fixed them all. > > api.py: > 495 , 552 No need for \'s when it's enclosed in ()'s. I think we > standardized on using \'s instead of ()'s in these kinds of cases, but > someone else can say for certain. > i've gone ahead and removed the parens. > pkgplan.py: > 73, 83: setting a default variable value of [] is almost never what you > actually want to do. I suspect you want EmptyI from pkg.misc as an empty > iterator. I also don't see why propose_reinstall is taking actions as an > argument at all, it doesn't seem to ever be used, it seems like it's all > in propose_repair. > your right. i've eliminated the actions argument from propose_reinstall(). i've also eliminated the "=[]" initialization for the actions parameter in propose_repair(), which basically restores the function arguments to what they were. > 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?) > 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. ;) > 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. thanks for looking at this. ed _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
