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 > Thanks for the review and general comments Shawn. To save eyestrain, if I've removed something it means "I agree" or "I've fixed it".
> client.py > > line 68: Why aren't the search errors part of the api exceptions now > as well? The short answer is that most probably will be eventually. The problem is that part of search is inside the API (the automatic indexing during install/uninstall/etc..) and part is outside "search" and "rebuild-index". When those all live inside the API, I'll move the exceptions as well. > > lines 397, 520: This should probably be part of the api now (not > sure where to put it) instead of tweaking this class directly. That > would also make the import on line 63 unnecessary. Good point, I hadn't even looked at those lines yet. I'm happy to throw those into the API for v1. > > line 416: "We are the knights who say Ni!" :-) > > line 439: Perhaps this call to display_pce should be a print that > takes advantage of a __str__ method of PlanCreationException? Ok, I'll try that. > > line 854: Looks like you could use misc.bytes_to_str here. Which > would also mean you could remove line 862. To be clear, using misc here bothers me, for all the reasons I argued against the GUI using it. I think we need to make a clearer decision about how misc is going to be used. For now, I'll change it as you've suggested since (I believe) the GUI went with a similar use. This does change the output slightly, which is probably a good thing, just want to note that a change in format happens. > > line 1476: This doesn't appear to be valid anymore? I have no idea what that references, so I'll remove it unless others object/can explain why it should remain. > > line 1534: --no-refresh is still a valid option, why remove this? Because I mismerged. Same thing applies to image-create. Which means a test case needs to be added to test image-create with no-refresh. > > 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. > > 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. > > api.py: > > line 2ff: I'd turn most of your comments after the def blah(...) > into docstrings for the related functions. docstrings for all your > classes would be nice too. Yeah, I'll put in doc strings. > > line 72: Shouldn't this be an "else:" case for line 74 to avoid a > double load of catalogs in the event they're refreshed? Nope. I actually had a similar question when I moved the code in here. J explained that the first load allows an incremental refresh of the catalogs. > > 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. > > api_errors.py: > > line 1ff: missing cddl header block and docstrings for all your > classes / functions would be appreciated. I'll add docstrings for those exceptions which are new or "mine", but will rely on others to fill in the docstrings of exceptions I simply relocated. > > 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. > > > image.py: > > lines 2037, 2038: drop the commented lines Not until I have a replacement for them. > > misc.py: > lines 314, 319: why are these commented? because I had forgotten to remove them after I was done testing them I've also updated the docstring to reflect the different functionality. > > ===================== > > Overall, I'm really happy with how this turned out. Bravo! Thanks :-) > > I just want to be clear that I was never against having a "refined API". > > My belief is that we have two apis in our gate: a "blessed API" and a > set of "private APIs". > > In my opinion, all APIs should work for all code in our gate. > However, code outside of our gate should only use "blessed APIs". > > This means to me that yes, any code that calls uses any of our private > APIs that is in our gate should be updated to reflect changes being > made in those private APIs. However, usage of private APIs by client > code in our gate should be strongly discouraged. > > As such, I'm looking forward to seeing this API you've taken an > initial stab at expanded in the future to be more inclusive. > > Cheers, _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
