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() > 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) > - line 131: if this function isn't used outside this module, then you can > just prefix it with a double underscore for privacy. > > I also wouldn't call it "..._decorator"; it's like calling all your > functions "..._function". > done. > 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. > nrlock.py: > > - line 58: docstring is identical to the one for locked(); probably could > have a couple of words added. > actually, locked_by_curthread() is unused so i've just removed it. > pkgremote.py: > > - Perhaps async_call_1 and async_call_2 (& related) should be async_call > and async_waiter? Or something that doesn't make it seem like you've > only got two things going on at once? > > - 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.) ed _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
