Hi Shawn, Thanks for the follow up comments. I've fixed them as indicated if there's no follow-up.
> src/modules/client/api_errors.py: > lines 28, 30, 31: these imports are unused now because of transport > changes > > line 320: shouldn't this be returned instead of 's =' ? Yes, you're correct. > src/modules/client/transport/engine.py: > line 255: tx here clashes with your import named the same thing, maybe > safer to use a different name? Thanks. I'll change this to something else. > line 424: str is a python built-in function, better to use a different > name? Thanks, changed. > line 516: where is data defined? did you mean treq.data? Yes. That's a leftover from converting from a tuple to the TransportRequest object. > line 552: could be a function I don't understand this comment. Would you please clarify? Line 552 is the definition for the function __teardown_handle(). > line 608: ultot, ulcur don't appear to be used anywhere The framework specifies what arguments the callback must accept. In this case I'm not using them, but the caller is supplying them, regardless. > src/modules/client/transport/exception.py: > line 59: __cmp__ should exist as an abstract method that raises > NotImplemented in api_errors.TransportError for consistency with line 65? There's a default __cmp__ that comes with the underlying object. I don't think we need to make this an abstract method. In the worst case, we compare by object identity and no harm is done. > lines 296-297: where is .data defined? I don't see it in the __init__ > for InvalidContentException, TransportException, or > api_errors.TransportError. Should this be hasattr(self, "data") and > self.data? I this should be self.reason, thanks for catching that. > src/modules/client/transport/fileobj.py: > line 28: unused import The version of the webrev that I'm looking at doesn't have an import here. > line 58: could be a staticmethod This is the flush() method, which I kept around so this object has a file-like object interface. > src/modules/client/transport/repo.py: > line 305: isn't __repo_cache redundant on a RepoCache object? What > about __cache instead? :) Changed. > line 307: missing self? Yes, fixed. > src/modules/client/transport/transport.py: > lines 136, 334: dir is a python built-in function, maybe use a > different name? Yes, changed. > line 218: you assign this, but never use it? I may have thought I was going to catch another error here. Removed. > line 664: where is the import for zlib? Rhetorical question? :P I've added one. Thanks for the additional comments. I'm going to work through the other feeback that came in this morning and then I'll push a new webrev. -j _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
