Tom Mueller (pkg-discuss) wrote: > 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. > I agree. I'm open to better suggestions. > - 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. > Why would there be a plan object in the API that the outside world should know about? You tell the API to make a plan. If you want to, you can ask for a description of what that plan will do (which gives the PlanDescription). Hiding the plan is one way we're refactoring the interfaces to present a consistent and simple interface to the client. If you can present an example where the client needs to modify the plan, I'd be very interested to hear it. > 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". > Is this an objection to the naming of "api.py"? If so, I'd happily take suggestions. I'm mildly against moving things into __init__.py as I think it's a fairly python specific construction which provides a needless barrier to entry to others coming to use our code, something I'd suggest is something an API should avoid where possible.
Also, the argument that others don't do this isn't, by itself tremendously convincing to me. Certainly, I'll take another look at how those programs are structured, but because CherryPy or PyOpenSSL do things one way doesn't mean it's the right way for us to do it. > 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 decision about how to expose Fmris, Versions, and Dot sequencies hasn't been made yet. Mostly because I don't have a strong feeling yet about what the right answer is. As answer to this and many of your subsequent questions: this is an API in developement, really, an intermediate API. Things will change. Things that are exposed may become hidden, and things that are hidden may become exposed as I discover information insufficiencies in the API. (For example, refresh currently returns an actual image. It does this because, given the other functions implemented, there would be no way for the GUI to update it's information after a refresh without rereading from disk, something that seemed silly to do since that information was already in memory. So, for now, it returns the image, a clear violation of the spirit of the API, and something that will be changed once it's more fully functional.) > - 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.) > Yes, as I've said previously in email here, the ProgressTracker is part of the API. Can you present a use case for A) Why you'd want to switch progress trackers partway through an operation B) Why you couldn't just make your own hybrid of the progress tracker which does what you want it to? > - how many other internal classes are exposed through this API > I have no idea right now. Probably a lot given that this is still a work in process. > 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? > No, the problem with the existing API is not that there are too many private methods. But if you'd be happier, I can probably move __licenses into image.py. It ended up there probably because, originally, licenses was an API function, which became a private method when I reorganized some other things. Despite that, yes, the API will have private methods. If clients call private methods they're unsupported, just like if function in image.py decided to call _write_main_dict_line in indexer.py directly, they'd be unsupported. > 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 data would you like it to return? > 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. > The point of locking is so that, for example, multiple GUI threads can't start planning an install, an uninstall, and an image_update all at the same time on the same PkgClient object, for example. Can this become more finely grained in the future? Probably. But for now, they help make the code thread safe, something that's important for the GUI. When we have the more general mechanism, perhaps, and only perhaps, it can replace the mechanisms here. > At what point would you expect the imports of the other pkg.client.* and > pkg.* modules to be removed from client.py? When the API is complete, and not in development. > (before or after the first > checkin of this API?) After, since I foresee at least 2 stages of commits, and probably 3 > 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. > So, what you'd like is for me to do all the work needed to completely analyze what the final API would look like (which basically means creating and implementing the final API since, at least for me, that's the only way to be certain I've gotten things correct). That's fine, but there's little chance that can be finished in time for the GUI team to move to it for the november release. Until I receive firm direction that we've changed our schedule on this, I'm going to proceed as I've planned. I'll make one other point, if this option, once it's fully in place, is truly so terrible that we as a team "can't live with it", then we can blow it up and someone else can try again. All that we'll have lost is the time I've spent on it (and the amount of time we've spent kvetching about 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 > _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
