Brock Pytlik wrote: > Shawn Walker wrote: >> Brock Pytlik wrote: >>> Here's the link: >>> http://cr.opensolaris.org/~bpytlik/ips-api-v1/ >>> >>> I haven't done as much cleanup as I usually try to do, but since I'm >>> ooto tomorrow, I thought it would be better to get this out now, >>> rather than next Monday. >>> >>> Other things that I know need to be improved, but that I think can >>> wait for another round of revisions: >>> API Exception organization >>> More complete functionality >>> Automated testing of cancellation (I hope to work with the GUI team >>> to test this by hand in the interim) >>> Versioning of the Progress tracker (perhaps) >>> More Documentation >>> Figure out the future approach to testing we'll take >>
>> lines 1668, 1672, 1674, 1676, 1684: You should pass img.imgdir here >> instead of mydir as the image object could have a different value for >> img.imgdir (see lines 190-191 of modules/client/image.py). > I'm pretty sure I want to be passing in mydir since, eventually, client > won't know about Image's at all. Can you help me understand how the > existing code could cause problems, and in what ways the new code would > blow up? What I might do is pass in a API object, instead of the dir. Because the find_root changes the effective value of mydir. If you read through find root, you'll notice it uses realpath(), etc. to ensure relative paths get transformed and so forth. In short, I don't think you should be using mydir as that is possibly a relative path or isn't necessarily the *real* image directory. We could change the logic for an image directory later... You should probably use an api object here or you need to pass img.imgdir. What you have currently, in theory, could regress some of the fixes that went into related to relative paths and so on. >> line 1713: it seems like we should call api.cancel() here in the >> event of a KeyboardInterrupt or a KeyboardInterrupt should >> automatically trigger a cancel in the api itself somehow. > I don't think I agree, at least not right now. For starters, > api.cancel() is a blocking operation, so it would delay the program from > actually exiting. Also, at least until there's on-disk state it'd be > necessary to clean up, I'd tend to pass. On the other hand, if you're > saying we should change our user interactions so that, for example, > hitting ctrl-c while the package is actually being installed should > simply ignore the users wishes, then I'd be more receptive to this change. I'm not sure what the final implementation should look like, I'm just concerned that a user triggering a KeyboardInterrupt doesn't cause an api.cancel(), and therefore, could prevent a proper cleanup of the operation. I don't think we want to ignore the user's wishes as that's likely just to make them use "kill -9". >> line 591: UNINSTALLED doesn't seem right here. I'd use the states >> presented in doc/pkg-states.txt, which in this case would appear to be >> DELETED if it was previously installed or IN_CATALOG if it's never >> been installed. > For now, I'm going to stick with Uninstalled since that's the info that > the client needs. Also, I'm not aware of a way to retrieve information > to determine whether a package is DELETED or IN_CATALOG. If we have that > info somewhere, I'll change the states as you suggest. We don't have that information right now, but please don't use UNINSTALLED. It's, in my view, inaccurate and misleading -- consider NOT_INSTALLED as a viable alternative. >> line 1: anything that inherits from exception should have it's own >> __init__ or have its own __init__ and call exception.__init__ > Init isn't inherited from Exception? If so, why should I create an init > method which only does Exception.__init__(self). Doesn't the way OO > works make that happen anyway? I wasn't aware __init__ had different > semantics for inheritance than other methods. Sorry, ignore this then. This was my mis-reading of the related Python docs [1]. Cheers, -- Shawn Walker [1] http://docs.python.org/tut/node10.html _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
