[UI][Common] Add daemon version check #427
Conversation
DjLegolas
force-pushed
the
Feature/add-daemon-version-to-ui
branch
from
548a3c0
to
a9044df
Compare
|
@cas-- - this is going to be a dependency for some other MRs, so it will be best to merge this first. |
deluge/ui/client.py
Outdated
|
|
||
| return '' | ||
|
|
||
| def is_daemon_version_equal_or_greater(self, version_check): |
There was a problem hiding this comment.
We can improve on the naming here and simplify it.
The expected use-case is new core functionality usage by the client so that could either be the latest daemon version or less likely a previous version. So the parameter can be an optional version where we use get_version() as default.
From there the method naming could be improved to is_daemon_compatible unless you have other suggestions?
deluge/ui/client.py
Outdated
| return '' | ||
|
|
||
| def is_daemon_version_equal_or_greater(self, version_check): | ||
| if VersionSplit(version_check) and self.connected(): |
There was a problem hiding this comment.
We shouldn't need these checks since VersionSplit will always return True or TypeError and calling above function does a connected check. Instead I would add a guard clause:
if not (daemon_version and version):
return False
deluge/ui/client.py
Outdated
| @@ -741,6 +743,25 @@ def connection_info(self): | |||
|
|
|||
| return None | |||
|
|
|||
| def connection_version(self): | |||
There was a problem hiding this comment.
I feel this is ambiguous method name since it is not clearly defining whether this is daemon or client version. I think would be better as daemon_version and probably nice as a property.
I know that using connection ties into connection_info but that also feels rather vague, especially since we get version using daemon.info RPC call...
deluge/ui/client.py
Outdated
| if self.connected(): | ||
| return self._daemon_proxy.daemon_info | ||
|
|
||
| return '' |
There was a problem hiding this comment.
Can be a one line return:
| if self.connected(): | |
| return self._daemon_proxy.daemon_info | |
| return '' | |
| return self._daemon_proxy.daemon_info if self.connected() else '' |
deluge/ui/client.py
Outdated
| Get the connected daemon version | ||
|
|
||
| Returns: | ||
| str: the daemon version |
There was a problem hiding this comment.
The str type should be moved to function definition
For new UI features to be added, one should make sure the backend daemon is supported and add fallback in case it doesn't. Here we add the ability to get the daemon version from the `Client` class and also check the version against a desired version.
DjLegolas
force-pushed
the
Feature/add-daemon-version-to-ui
branch
from
a9044df
to
d60ccf0
Compare
|
Don't know why I'm responding to each message when I can just write here - all changed as suggested :) |
|
Thanks for this! I did change my mind about the naming of the function and settled on |
For new UI features to be added, one should make sure the backend daemon is supported and add fallback in case it doesn't. Here we add the ability to get the daemon version from the `Client` class and also check the version against a desired version. Closes: deluge-torrent#427
For new UI features to be added, one should make sure the backend daemon is supported and add fallback in case it doesn't.
Here we add the ability to get the daemon version from the
Clientclass and also check the version against a desired version.