Code review comment for lp:~alecu/ubuntuone-client/proxy-tunnel-useit

Revision history for this message
Alejandro J. Cura (alecu) wrote :

> * Could you please change this assert:
>
> self.assertTrue(self.spawned, "The tunnel process is started.")
>
> to something that reflects that self.spawned is not a boolean but a list?
> Perhaps using assertEqual against what should actually be in the list.

Good point, I'll fix it.

> * This is in live code, TUNNEL_PORT = "Tunnel port", is it correct? does no
> look like a port to me :-/

The string is correct.
But I agree that the name of the constant is misleading, so I'm renaming it to something better.

> * This import is not alphabetically ordered: from ubuntuone.proxy import
> tunnel_client. Also, if I understand correctly, that import can fail with an
> ImportError if the user does not have the -proxy-support debian/ubuntu package
> installed, since the whole ubuntuone.proxy namespace will be distributed in
> another binary package (as far as I know).

The tunnel_client.py module should not be part of the different binary package, only tunnel_server.py and bin/ubuntuone-proxy-tunnel depend on qtnet and should be in a different package. Also tunnel_client has code to deal with the case where bin/ubuntuone-proxy-tunnel is not installed or cannot be found, and in that case SD starts just like if no proxies were configured.

« Back to merge proposal