Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use QgsNetworkAccessManager instead of urllib for network requests #2299

Closed
wants to merge 1 commit into from

Conversation

ltbam
Copy link

@ltbam ltbam commented Sep 9, 2015

  • Use of QNetworkManager instead of urllib for network access (proxy support).
  • Fixed some UTF encoding issues
  • Added some WMS types for link recognition.

@m-kuhn
Copy link
Member

m-kuhn commented Sep 9, 2015

Assigned @tomkralidis

l = []
data = data.items()
for k, v in data:
k = str(k)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

str should not be used for strings.

  • Remove it if you are sure that there is a string returned always anyway
  • Change it to unicode() if you need to convert it to a string

@m-kuhn
Copy link
Member

m-kuhn commented Sep 14, 2015

I think owslib is an external dependency. Patching it makes it harder to maintain it (i.e. if it's updated to a newer version the patch will need to be re-applied).

I would like to get @tomkralidis feedback on this.


Alternatively, would it be possible to set environment variables for proxies to get urllib working properly? This approach would fix all occurrences of urllib and leave the dependency code untouched.

@strk strk added the Requires Changes! Waiting on the submitter to make requested changes label Sep 17, 2015
@tomkralidis
Copy link
Contributor

OWSLib is an external dependency that we will not patch here. If there are OWSLib specific changes required, this is better realized by making them in OWSLib proper. Note that the goal is for OWSLib to be completely upstream (and outside) of QGIS proper.

I would recommend this PR be closed and a new PR issued with only MetaSearch specific changes.

@m-kuhn
Copy link
Member

m-kuhn commented Sep 29, 2015

@tomkralidis what do you think about the environment variables for urllib approach?
That should not need upstream adjustments as far as I can tell.

@tomkralidis
Copy link
Contributor

@m-kuhn +1 for use of environment variables (http_proxy, I'm guessing).

@m-kuhn
Copy link
Member

m-kuhn commented Sep 30, 2015

@ltbam could you implement these changes?

@m-kuhn
Copy link
Member

m-kuhn commented Nov 23, 2015

@ltbam any feedback on this?

@m-kuhn
Copy link
Member

m-kuhn commented Dec 15, 2015

Closed due to lack of feedback. Will be great if somebody will be able to implement this.

@m-kuhn m-kuhn closed this Dec 15, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Requires Changes! Waiting on the submitter to make requested changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants