-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
PR: Use requests
instead of urllib
for updates
#21423
Conversation
Hello @mrclary! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2023-10-17 19:39:18 UTC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mrclary ! Left some suggestions regarding the error text when doing the initial check for updates (keep mentioning that the error is related to Spyder checking for updates). I'm missing to give this a manual check but seems like it should work 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some small suggestions for you @mrclary, the rest looks good to me.
Also, could you add a test for WorkerDownloadInstaller
? That can be similar to the ones we have for WorkerUpdates
and could also test the progress reporting through the sig_download_progress
signal.
Note that the default is to verify ssl certificates.
…hetical constructs.
Co-authored-by: Daniel Althviz Moré <16781833+dalthviz@users.noreply.github.com>
Co-authored-by: Carlos Cordoba <ccordoba12@gmail.com>
Actually, I think the only things to test in the Do you have a recommendation on how to go about this? I was thinking of three options:
|
Checking this locally I noticed a couple of things: Status bar text is not being updated when dismissing update:
Status bar text is not being updated in case an error happens while checking updates (no internet connection):Seems like this was happening also before so just sharing (could be worthy to fix here or maybe in a follow-up PR after this one gets merged).
Status bar text is being wrongly updated in case case an error happens while downloading the installer (no internet connection):Seems like this was happening also before so again just sharing (as the issue above, it could be worthy to fix here or maybe in a follow-up PR after this one gets merged). |
@dalthviz, thanks for reporting. Yes, I believe these may have been occurring before this PR. I recommend that we fix those in a separate PR. I may be able to address them next week before 5.5.0 release. |
4690ba0
to
314c3ea
Compare
Per discussion with @ccordoba12, we will not include unit testing for the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mrclary!
page = requests.get(self.url) | ||
data = page.json() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the expected page is not returned, we should raise a HTTPError as soon as possible, instead of a much longer and more cryptic JSONDecodeError
on the next line or later on. This is much clearer and friendlier for users and ensures their reports will be much clearer and more useful for us.
So, in a followup PR we should add here between the .get()
and the .json()
page.raise_for_status()
Which will raise this error appropriately.
except ConnectionError as err: | ||
error_msg = CONNECT_ERROR_MSG | ||
logger.debug(err, stack_info=True) | ||
except Exception as err: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could add one more block here to catch HTTPError
:
except HTTPError as err:
error_msg = HTTP_ERROR_MSG.format(page.status_code)
...
with a friendly message, e.g. :
HTTP_ERROR_MSG = _(
'HTTP error {status_code} when checking for updates.'
'<br><br>Make sure your connection is working properly.'
)
) | ||
|
||
CONNECT_ERROR_MSG = _( | ||
'Unable to connect to the internet while checking for Spyder updates.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should say something more like "Unable to connect to the Spyder update service." since this could be something specific to that site or due to a firewall, proxy, etc. issue rather than just internet connectivity
@CAM-Gerlach, I'm sorry, I should have requested a review from you as well. I'll submit a follow-up PR with your suggestions. |
No worries, I was away on my trip to the Python core dev sprint until yesterday, so I wasn't around anyway. |
Description of Changes
urllib.urlopen
andurllib.urlrequest
withrequests.get
when checking and downloading updates.Issue(s) Resolved
Fixes #21398