Edward Pilatowicz wrote: > On Mon, Jun 18, 2012 at 01:15:04PM -0700, Danek Duvall wrote: > > Edward Pilatowicz wrote: > > > > > > > > https://cr.opensolaris.org/action/browse/pkg/edp/pkg.pli.cr3/webrev.cr1.diff > > > > Mostly nits. > > > > client.py: > > > > - line 1691: do we clean up the saved plan if there's nothingtodo()? > > > > no. but i thought this would be ok because: > > - it's a tiny file in a temporary directory that is going to get > overwritten during the next execution > > - it allows us to test re-loading, preparation, and execution, of a plan > in all cases (even if we have an empty plan). i decided this was ok > because nothing in the API prevents someone from preparing/executing a > NOP plan, so i wanted to make sure that this also worked with staged > execution. i actually created a test case to verify this scenario, > see test_staged_noop()
Okay. Might be nice to have a comment there saying this is intentional, and why. > > client/api.py: > > > > - line 99: if we're not using PlanDescription, why do the import, rather > > than disabling the lint warning? Only place I see PlanDescription is > > on lin 1147, where we refer to it as plandesc.PlanDescription, so we > > probably actually don't need this import. > > > > because PlanDescription is part of the public API, so i thought folks > should be able to continue to get access to this object by just > importing pkg.client.api (so they can do things like > help(pkg.client.api.PlanDescription) I saw the comment in the other place you disabled this warning, didn't think it applied here. Thanks. > > But I wonder if it should look more like the _LockedCancelable class > > (I'm not sure why that was implemented as a class rather than a > > function), or whether you should go even further and simply use the > > _LockedCancelable class, possibly modifying the __call__() method to > > give it different behavior in some circumstances. > > > > i structured this function based on other code that grabs the activity > and image lock. for example __plan_common_start(). > > _LockedGenerator() has a bit more complexity and it's comments say that > it's designed to wrap generator functions it's wrapper routine invokes > yield() but i need my routine to invoke return, and i've had very bad > experiences debugging functions that mix yield and return. to mitigate > risk i'd like to leave this as is for now. Okay. Though I didn't say _LockedGenerator, I said _LockedCancelable, which does return and not yield. > > pkgremote.py: > > > > - line 231ff: these will all end up printing out "None is None", which > > doesn't seem useful. > > if the assert fails that means self.__pkg_op is not none, so we'll print > out <something> is None. (which will clearly not be true.) Duh. Okay. Thanks, Danek _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
