Thanks for the in-depth review! Responses below.. [EMAIL PROTECTED] wrote: > I also finally got a chance to look at this code. I've included my > comments below. > > Meta comment about OSBase: > > It's incredibly cumbersome to call a static method of get_instance() > every time we want to look a the attributes for the current OSBase. > This isn't going to change during the runtime of the script. Instead of > calling OSBase.get_instance().<method>, create an OSBase instance when > the client/depot are initialized. Then callers can simply to > ob.<method> instead of the long, convoluted approach that is currently > used.
Agreed. I'll get rid of the get_instance() thing and just do once, at initialization. > directory.py:107 - errno can only return one error code at a time, so > this conditional is impossible. I think you meant if e.errno != > errno.EEXIST or e.errno != errno.ENOTEMPTY I observed your followup and will ignore this one.. > generic.py:146 - Why are we introducing a file_closer()? And, more > importantly, why does the file closer perform an unlink instead of a > close in this case? The issue is that the old code took advantage of the Unix-specific behavior that if you open() file, and immediately unlink() it, you can still read() from the file descriptor and get the contents of the file - the file isn't actually removed from the filesystem until the last descriptor pointing to it is closed. On Windows, it doesn't work like that - the unlink() immediately fails with an Exception on a file that has been open()ed, and you can't unlink() it until all file descriptors are close()d. So, the file_closer() is there to clean up the temporary file created by generic action class, once the action has read the data from the temporary file (in the file action). > filelist.py:190 - Why did you change make_opener() from a private static > method to a public instance method? Is this method being called from > outside the filelist code? The make_opener function isn't doing > anything with the object that it's operating on. It looks to me like > both make_opener and make_closer should remain private static methods. My bad - during porting in this area, I was discussing it with Danek and he suggested that perhaps the class should keep track of the files it opens by maintaining a list within an instance variable, and then cleaning up the list in the class's destructor. Turns out the python runtime isn't guaranteed to call a class's destructor at all (and it wasn't about half the time in my initial tests), so that turned out to be a dead end, but I forgot to change it back to a static method. I'll change it back. > transaction.py:231 - You need to send the client an error code here. My > guess is that 501 - "Not Implemented" is appropriate, but Danek may have > other suggestions. > OK. I'll go with 501 for now. > Meta comment about os_util: > > Do you need to include a wrapper for shutil in this or OSBase? The > shutil documentation says that, "on MacOS the resource fork and other > metadata are not used. For file copies, this means that resources will > be lost and file type and creator codes will not be correct." > > We use shutil copy and rmtree, and perhaps other functions too. I'm > pretty sure that we want these to work correctly across a variety of > platforms. Hmm.. well, from our (Update Center 2.0 project) immediate needs, we don't need this, but I see what you mean. Since the only caveat is for file copies, and the only copy API we use from shutil is copy(), then this is the only one that needs special treatment (such that it defaults to shutil except on MacOS, where it uses macostools.copy()). Nice catch. -jhf- _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
