[pkg-discuss] Code Review: New Transport

[johansen at sun.com]

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