Code review comment for lp:~vila/bzr/186920-lp-proxy

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Vincent Ladeuil wrote:
> Vincent Ladeuil has proposed merging lp:~vila/bzr/186920-lp-proxy into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
>
>
> Thanks to Gordon Tyler for starting a proof-of-concept implementation, I've finally
> been able to fix bug #186920 et all (the one about lp URLS that can't be
> resolved behind proxies).
>
> This patch implement an XMLRPC transport class using our _urllib2_wrappers (the fundations
> for the urllib http client), that gives us proxy support for free.
>
> I still haven't implemented a true proxy test server but I've added tests that exercise
> the full urllib http stack.
>

+class Transport(xmlrpclib.Transport):
+
+ def __init__(self, scheme, use_datetime=0):

^- Shouldn't we have our own name here? We *can* re-use the existing
name, but it is potentially confusing.

Especially given that we have a "Transport" hierarchy already (and not
to mention that so does Paramiko), I'd like to at least call it
"XMLRPCTransport".

Also, it seems that Gordon responded that there was a problem with not
passing the original port to the proxy? Did you fix that in this? Or did
his comment come after the merge proposal?

I assume that the http/https trick is this line:

+ self._opener = _urllib2_wrappers.Opener()

(Opener supposedly creates the right instance of HTTP or HTTPS based on
the url/the url of the proxy?) I assume that is why you don't need:
- - if uri_type == 'https':
- - transport = xmlrpclib.SafeTransport()
- - else:
- - transport = xmlrpclib.Transport()
+ transport = Transport(uri_type)

anymore.

+
+ self.wfile.write(tcs.canned_response)
+
+class PreCannedServerMixin(object):

^- bit of whitespace here

Otherwise:

  review: approve

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkrrGn4ACgkQJdeBCYSNAAN9SQCdFnI2kiY1Pykdz2xEMmQtYVBZ
VZ0An3B1OXrswFOFltoF0851r217zJiF
=Vmrj
-----END PGP SIGNATURE-----