> 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.
> Everything looks good, here are some small comments: QNetworkProxy. NoProxy) QNetworkProxy. DefaultProxy) . Returning default proxy will make
>
> return QNetworkProxy(
>
> 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(
> 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 hostName( ))
> a QString in memory, it would be as simple as changing:
>
> 340 + self.proxy_domain = proxy.hostName()
>
> for
>
> 340 + self.proxy_domain = str(proxy.
>
> 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!