-
-
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
Add a check for updates method #2321
Conversation
@@ -942,6 +948,12 @@ def create_edit_action(text, tr_text, icon_name): | |||
support_action = create_action(self, | |||
_("Spyder support..."), | |||
triggered=self.google_group) | |||
check_updates_trig = lambda: self.check_updates(True) | |||
check_updates_action = create_action(self, |
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.
I think you should use here self.check_updates_action
directly and remove the line below.
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.
Ok
Thanks for working on this!! It's really useful. About your todo's:
|
@blink1073 being the closest thing to a native speaker 😸, could you check if the messages (in the description) make sense to you? Thanks |
@ccordoba12, what about release candidates? Will those be listed as well in the releases page? I have not taken any measures to test for those. |
👍 from the 'Murican |
if update_available: | ||
msg = _('Spyder {0} is available. <br><br>Please use your ' | ||
'package manager to update Spyder or go to our ' | ||
'<a href="{1}">Releases</a> page to download Spyder ' |
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.
To not be too repetitive, please change this text to
'<a href="{1}">Releases</a> page to download this '
'new version'
Also, please use double quotes to build this whole string. It's better because using single quotes in written English is quite common (while not so much for double ones).
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.
You know what would be cool here too? To add a link to the Installation page in our documentation. It could something like
If you don't know to update Spyder, please refer to our Installation instructions.
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.
Ok 👍
@ccordoba12 what should be default behavior when checking with betas or release candidates? If the newest is a beta (or rc), ignore it and check the next one until an official non beta release is found and compare to that? |
I think that at first we should just warn about stable versions. If we include beta or rc versions, sit should be an option, disabled by default. |
There are too many use cases to decide when it makes sense to have it on or off... package managers in linux distros.. pips condas.. python xy... winpython etc... so I think it is still sensible to have ON by default and only check against stable versions. The message box can include a checkbox and ask the user, if he wants to continue receiving this messages... that would make it more obvious and easy to correct. What do you think? |
The "off by default" part is about beta versions. Of course the check about stable versions should be on. Including a checkbox in the message box in addition to the preferences is OK for me. |
Yes @Nodd , now I understood what you meant. Will make the changes soon. @ccordoba12 just to confirm, are these the expected tags (structure in downward order of release) to be found in Spyder releases:
|
@Nodd @ccordoba12, take a look at the description for the updated look. Should all the other boxes contain the checkbox asking? (Error box, or Uptodate Box?) |
Ok so this is the logic I coded
Comments? |
I also added the extra needed logic inside the Worker class, but now I think the worker should go out of the |
@@ -190,7 +190,9 @@ def is_ubuntu(): | |||
'cpu_usage/timeout': 2000, | |||
'use_custom_margin': True, | |||
'custom_margin': 0, | |||
'show_internal_console_if_traceback': True | |||
'show_internal_console_if_traceback': True, | |||
|
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.
Please remove this blank line
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.
Ok 👍
return (False, latest_release) | ||
|
||
if self.is_stable_version(latest) and \ | ||
version.startswith(latest) and latest != version: |
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 line should be indented only two spaces, i.e. like this
if self.is_stable_version(latest) and \
version.startswith(latest) and latest != version:
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.
ok
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.
When I do this, pep8 complains... if I use 8 spaces, pep8 does not complain...
@ccordoba12 made suggested changes. |
|
||
# check_version is based on LooseVersion, so a small hack is needed so | ||
# that LooseVersion understands that '3.0.0' is in fact bigger than | ||
# '3.0.0rc1' |
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.
One last thing: please move this hack to check_version
. It'll be more useful there :-)
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.
Done
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.
Bear in mind, that also, is_stable_version
will be moved to utils.programs
Another thing: are you squashing commits? I don't like that too much because it doesn't let us see the evolution of the PR in |
For small changes, having too many commits seems messy, but for this one you are right. I will stop squashing, when a PR is relatively big like this one. |
@ccordoba12 added latest suggestions. |
feedback = True | ||
elif self.thread_updates is not None: | ||
self.thread_updates.terminate() | ||
feedback = True |
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.
I don't feel at ease with this feedback
variable. It's passed to the worker, but it doesn't do anything with it. It seems the worker just holds it, so that _check_updates_ready
can later read it.
But wouldn't it be better to move this block to _check_updates_ready
directly and get rid of this passing around process?
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.
Yes, you are right, these are the leftovers of the original way I did it, when the method accepted parameters. Will move it
@ccordoba12 I think this one is good to go :-) |
self.check_updates_action = None | ||
self.thread_updates = None | ||
self.worker_updates = None | ||
self.flag_updates_feedback = True |
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.
Sorry, last thing :-) I don't like too much this variable name.
Suggestions:
self.updates_feedback
self.give_updates_feedback
I think any of those would convey its meaning better ;-)
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.
👍
Next... :-)? |
Wait for Travis ;-) |
;- | |
I just tested it locally and it's working as expected :-) This is great addition, thanks a lot for working on it!! |
Add a check for updates method
👍 in the release candidates we will check for sure if it is working as it should |
Hi all, I discovered recently that:
Would it be possible to set this option to "no" by default ? |
Fixes #2307
(Note from @ccordoba12: Please leave Fixes instead of Implements because that way when I merge the PR, GH will automatically close the corresponding issue. If you don't like Fixes, here are the other words you can use to make this happen)
Description
Add a method to check for updates querying the Github API, using a QThread and Worker to avoid blocking the GUI.
The method is called on startup if running from installed version and will only display a message if there is an update. If running in
DEV
mode versions it will not be triggered on startup.There is also an entry in the Help menu to check for updates at any moment, unlike the one on startup, this action will give feedback also if no updates are available, or if there is some internet error.
Messages
Ubuntu
Ubuntu
Windows
New option in the general preferences
This option is on by default and then the user can decide by checking or unchecking the checkbox.
Windows
Checklist