Brock, Please consider these comments of a more general nature (looks like Shawn got the detailed ones).
Please change the API to be more object oriented: - The term "PkgClient" doesn't represent what the thing is that is being manipulated here. The object here is an Image. If a client is going to manipulate multiple images at the same time, it would need multiple of these objects, but there is still only one client. - The PkgClient.describe method returns a PlanDescription, so it would seem that the thing being described in a "Plan" but there isn't any "Plan" object in the API. I have yet to see a Python library that has a module called "api". Can you provide an example of another Python library that uses this pattern? As a counter-example, consider the CherryPy and PyOpenSSL libraries that we use. CherryPy exposes the cherrypy and cherrypy.wsgiserver modules as public interfaces. PyOpenSSL exposes the OpenSSL and OpenSSL.crypto modules. Even for a large Python system like mercurial, there is no "api" module. One just uses "from mercurial import hg" when writing an extension to mercurial. Please rename the "api" module to represent what interface is actually being used. I'd suggest "pkg.client.image", but that is already taken. ;-) Maybe these classes could be moved into pkg/client/__init__.py in such a way that a programmer would just do "import pkg.client". There is insufficient encapsulation in the API. - the PackageInfo object exposes version.DotSequence objects. Is that intentional? i.e., is version.DotSequence part of the external API? - the PkgClient class requires a ProgressTracker object. So is this API making ProgressTracker a public interface? Also, if one wants to use different progress trackers for different operations, how does one do that? (for example, different progress trackers for loading the catalogs vs. making the install plan in the plan_install method.) - how many other internal classes are exposed through this API? Why does the API have "__" methods (like __licenses). Shouldn't this functionality be in the backend? Isn't the problem with the existing API that there are too many private methods? All exceptions should return data that can be extracted by the client. VersionException and several others are good examples of this. But ProblematicPermissionsIndexException doesn't do this. What is the intent of the __activity_lock functionality? We eventually need to have locking that prevents multiple clients (in separate processes) from modifying an image concurrently. Once we have that, the same functionality will also work for different threads in a single process. So thread-based locking is not really useful here. It's also not clear to me that this API layer should be responsible for locking, as that may not provide sufficient granularity. At what point would you expect the imports of the other pkg.client.* and pkg.* modules to be removed from client.py? (before or after the first checkin of this API?) I'd like to at least see a plan for how complicated this api module will get in order to be able to remove those imports, so that the true expense of this option will be understood before the team has to live with it. Thanks. Tom 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, > Brock > _______________________________________________ > pkg-discuss mailing list > [email protected] > http://mail.opensolaris.org/mailman/listinfo/pkg-discuss > _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
