[rfc] [patch] pycurl transport

[Martin Pool]

On 12 Jan 2006, Robert Collins <robertc at robertcollins.net> wrote:

Thanks for reviewing it.

> PEP8 - separate std lib, third party and internal imports, and put in
> alhpabetical order, and two lines of gap at the top level:

Thanks

> One line of VWS after the class before the first class scope item other
> than a docstring:
> 
> class PyCurlTransport(HttpTransportBase):
> 
>     def __init__(self, base):

This ugliness can be a reminder to always add a docstring. :-)

> > +    def get(self, relpath, decode=False):
> > +        if decode:
> > +            raise NotImplementedError
> > +        return self._get_url(self.abspath(relpath))
> 
> Whats decode here? Its not in the main transport api .... and having it
> raise makes it look rather - uhm - unneeded ?

It was present in HttpTransport.  Perhaps it's obsolete/unused; I'll
check.

> > +    def _get_url(self, abspath):
> > +        sio = StringIO()
> > +        # pycurl needs plain ascii
> 
> All the transport functions should only be getting ascii - though I
> haven't audited to make this the case yet. (They only work with URLs,
> all URLs are 7-bit safe.)
> 
> > +        if isinstance(abspath, unicode):
> > +            # XXX: HttpTransportBase.abspath should probably
> > url-escape
> > +            # unicode characters if any in the path - domain name
> > must be
> > +            # IDNA-escaped
> > +            abspath = abspath.encode('ascii')

It was getting a Unicode object when running the tests, which is why I 
added this.  Apparently it was only ever a Unicode containing ascii
characters though.  Perhaps I should change this to just raise an
assertion and fix the callers.

> .. These should go into mainline regardless - +1 on them ;)

OK, good.  The messages are much clearer, showing the method name and
instance class.

> this looks good +1 with the above tweaks ...
> 
> some small extra thoughts:
> perhaps it would be cleaner to do:
> mkdir bzrlib/transport/http
> bzr add bzrlib/transport/http
> bzr mv bzrlib/transport/http.py bzrlib/transport/http/__init__.py
> 
> Then, have _pycurl.py and _urllib.py containing the respective pycurl
> and urllib implementations, with the common code in __init__.py

Yes.  

I had avoided those names because of concern they would clash with the
possible C impl of pycurl, but I guess that will be imported from a
different module and so not really clash.

> As for which implementation is default, if we change the protocol
> handler to allow a *list* of handlers for a prefix, then we can just
> probe until one allows itself to be constructed, and if none do, then
> complain/provide the last error.

Yep.  I'll send a patch for that part for review.

-- 
Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060112/10926774/attachment.pgp