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

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

> Everything looks good, here are some small comments:
>
> return QNetworkProxy(QNetworkProxy.NoProxy)
>
> As far as I understand this, the tunnel will use no proxy ignoring the
> application level proxy. I think it might be more appropriate to return
> QNetworkProxy(QNetworkProxy.DefaultProxy). Returning default proxy will make
> the code use a higher alternative, for example the application-level proxy
> settings. I know that we are using build_proxy to set the application level
> proxy, but I want to be 100% saure that we do the right thing, also when the
> proxy is the application level proxy DefaultProxy and NoProxy have the same
> meaning therefore in our current code we will be using the same.

I've fixed this, thanks for the suggestion.

> Maybe we could ensure that self.proxy_domain is always a python string and not
> a QString in memory, it would be as simple as changing:
>
> 340 + self.proxy_domain = proxy.hostName()
>
> for
>
> 340 + self.proxy_domain = str(proxy.hostName())
>
> I think it would be less error prone, what is you opinion?

I don't think that's needed, since most uses of self.proxy_domain are for comparing again proxy.hostName(). In the only place where it's sent from the containing object to the keyring.get_credentials method it is already being transformed with str.

I'm now pushing the branch with the first fix requested.

Thanks for the review!

« Back to merge proposal