On Wed, Jul 14, 2010 at 02:03:24PM -0700, Danek Duvall wrote: > [email protected] wrote: > > > http://cr.opensolaris.org/~johansen/webrev-xpub-1/ > > transport/__init__.py: > > - Can you simply do > > from .transport import Transport, ImageTransportCfg, > GenericTransportCfg > > (and so on) or does that break badly? Or is there a reason you don't > want to do that?
I'm not sure. I found a recipe that did the imports this way. I'm not really certain what the rules for __init__.py are. I had reason to believe that this file was unusually magical. > transport/engine.py: > > - line 853: Why would you get EROFS when opening an object for reading? Uh, I think this was accidental paranoia. Removed. > - line 879: why wouldn't this be set by the READDATA line above? Not sure I understand the question, but I do see the problem. The assignment to hdl.r_fobj should happen before the call to READDATA, and r_fobj should be passed in instead. Will fix. Thanks for catching this. > transport/repo.py: > > - line 598, 625, et al: dict literals shouldn't have a space before the > colon. Fixed. > - line 1167: "tranasaction" -> "transaction" Found three instances of this. Thanks. > - line 1376: "it's" -> "its" Changed. > transport/transport.py: > > - line 146: return self.__policy_map.get(policy_name, False) Thanks, Shawn already had me fix this. > - line 151: double space Fixed. > - line 154: self.__publishers.pop(publisher_name, None) I assume you mean, return self.__publishers.pop(publisher_name, None)? Changed to that. > - Your model for intersecting multiple origins with publication is > interesting. I suppose it wouldn't actually happen very often, but if > it did, and you ended up sending different parts of a transaction to > different depots, things might get a little confused. Yes, you're correct. The current assumption is that publication clients won't create Publishers with multiple origins, unless they know what they're doing. > - line 2116: "an close" -> "a close" Changed. > misc.py: > > - line 376: what's this change intended to fix? Things may very well have > changed, but I would expect that if "f" is a file-like object, then it > should have "close()" called on it, too. This fixes a nasty and hard-to-locate bug where the depot will traceback if you perform multiple publication operations on the same connection. The caller should only close the fileobject if it opened it itself. In this particular case, it was closing a cherrypy fileobject that the server needed to keep open. > pull.py: > > - line 137: the "error" here will override the "error" function you use > on line 146. Thanks, fixed. > - line 139: why is this declaration needed? Ditto in other places where > these names are only being read and not assigned to. (Similarly in > importer.py and merge.py.) Removed. I misunderstood the rules for global. > - line 148: "% cache_dir" should be outside the _() Changed. > - line 396: is the try/except block unnecessary? (Same in importer.py, > line 1441.) No, I don't think it is. Thanks for catching this. > After seeing setup_publisher() and setup_transport() several times in the > various utilities, I wonder if they should be made convenience functions in > the client.transport module. Shawn suggested this too, but each of these routines, at least for setup_publisher(), is a little different. I'll see if there's a better way for me to make this common. No guarantees, though. -j _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
