Replace usage of connection proxy with the requests library. #3552
Conversation
Rather than just duplicating this logic.
…into remove-connection-proxy
| @@ -640,26 +630,20 @@ def log_imported_packages(self): | |||
| """ | |||
| PackageGatherer().log_imported_packages() | |||
|
|
|||
| def processVersion(self, version_info): | |||
| def processVersion(self, version_str): | |||
There was a problem hiding this comment.
version_str is the return value from get_current_release_version(), which returns a tuple of (str, str, Version).
Here, you're treating this tuple as a string? Or as a dict in line 646?
There was a problem hiding this comment.
version_info was originally a dictionary of the JSON fetched from the server. I was going to just make this method take in the string only, but it looks like I mistakenly read the return type of get_current_release_version as a string rather than a tuple. I've changed this so it takes the tuple in, and destructures it to get the necessary values.
|
|
||
| version_string = version_info["version"] | ||
| url = version_info["download_url"] | ||
|
|
||
| return version_string, url, parse(version_string) | ||
|
|
||
|
|
||
| except ConnectionError: |
There was a problem hiding this comment.
Shouldn't this be requests.exceptions.ConnectionError?
https://stackoverflow.com/questions/16511337/correct-way-to-try-except-using-python-requests-module
Also, what's the advantage of having a separate except for it if you're just dropping it completely?
There was a problem hiding this comment.
Yes looks like I didn't import it properly.
I was trying to mimic the original behaviour where it would just fail silently if it failed to establish a connection. But now you point this out, I don't think this is good behaviour. I'll just remove this handle so we just always fail with an error.
| logger.info("Connected to www.sasview.org. Received: %s", content) | ||
|
|
||
| version_info = json.loads(content) | ||
| response = requests.get(web.update_url) |
There was a problem hiding this comment.
Consider adding a timeout like you did before with config.UPDATE_TIMEOUT.
Otherwise, get will hang indefinitely since the default timeout is None.
There was a problem hiding this comment.
Good idea. I'll reuse the value we already had from the config.
|
Looks good, ready to merge after merge conflicts are resolved. |
Description
This PR aims to fix #3420 by removing our reliance on the
ConnectionProxyclass, and instead use thegetrequest function from therequestslibrary.How Has This Been Tested?
Tested on
run.py, and the wheel. Unfortunately, I haven't been able to test on the installer as I'm not sure if there is a way to fake the version to an older one to test the popup menu. Does anyone know how to do this? It might be useful if we add a debug version override flag for this purpose.Review Checklist:
Documentation (check at least one)
Installers
Licensing (untick if necessary)