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

Add schedule API and refactor info/config/stat apis in DownloadStation #3

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

sylvaindd
Copy link

No description provided.

@sylvaindd sylvaindd marked this pull request as draft November 27, 2020 15:19
@sylvaindd sylvaindd marked this pull request as ready for review November 27, 2020 15:27
@oncleben31 oncleben31 requested a review from Quentame December 17, 2020 22:46
Copy link

@mib1185 mib1185 left a comment

Choose a reason for hiding this comment

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

Hi @sylvaindd,
thanks for contributing 👍
Please rebase your branch to the current master and furthermore add a description to this PR, what does it do, what does it add.
Last but not least, please take care of the commits below.

regards,
Michael

Comment on lines +48 to +49
- repo: https://github.com/pre-commit/mirrors-prettier
rev: v2.2.0
Copy link

Choose a reason for hiding this comment

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

please rebase to current master branch

"""Return schedule configuration about the Download Station instance."""
return self._schedule_config

def set_schedule_config(self, enabled: bool = None, emule_enabled: bool = None):
Copy link

Choose a reason for hiding this comment

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

set defaults to False

@@ -32,17 +37,50 @@ def update(self):
self._tasks_by_id[task_data["id"]] = SynoDownloadTask(task_data)

# Global
def update_info(self):
Copy link

Choose a reason for hiding this comment

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

what is the intention of this dedicated update method?
Those the user is forced to do an manually initial update before info can be accessed

return self._dsm.get(self.INFO_API_KEY, "GetInfo")
return self._info

def update_config(self):
Copy link

Choose a reason for hiding this comment

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

what is the intention of this dedicated update method?
Those the user is forced to do an manually initial update before config data can be accessed


def update_config(self):
"""Update configuration about the Download Station instance."""
self._config = self._dsm.get(self.INFO_API_KEY, "GetConfig")["data"]
Copy link

Choose a reason for hiding this comment

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

  • this could cause KeyError, in case _dsm.get() was unsuccessful
  • prior result was not restricted to content of data

def update_schedule_config(self):
"""Update schedule configuration about the Download Station instance."""
self._schedule_config = self._dsm.get(self.SCHEDULE_API_KEY, "GetConfig")[
"data"
Copy link

Choose a reason for hiding this comment

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

this could cause KeyError, in case _dsm.get() was unsuccessful

return self._dsm.get(self.INFO_API_KEY, "GetConfig")
return self._config

def update_schedule_config(self):
Copy link

Choose a reason for hiding this comment

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

what is the intention of this dedicated update method?
Those the user is forced to do an manually initial update before schedule config data can be accessed

Comment on lines +154 to +155
# set_schedule_config(self, enabled: bool = None, emule_enabled: bool = None)
api.download_station.set_schedule_config(True, False)
Copy link

Choose a reason for hiding this comment

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

Suggested change
# set_schedule_config(self, enabled: bool = None, emule_enabled: bool = None)
api.download_station.set_schedule_config(True, False)
api.download_station.set_schedule_config(enabled=True, emule_enabled=False)

self.update_schedule_config()
return res

def update_stat(self):
Copy link

Choose a reason for hiding this comment

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

what is the intention of this dedicated update method?
Those the user is forced to do an manually initial update before stat data can be accessed


def update_stat(self):
"""Update statistic about the Download Station instance."""
self._stat = self._dsm.get(self.STAT_API_KEY, "GetInfo")["data"]
Copy link

Choose a reason for hiding this comment

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

  • this could cause KeyError, in case _dsm.get() was unsuccessful
  • prior result was not restricted to content of data

@mib1185 mib1185 requested a review from oncleben31 September 12, 2021 20:20
@mib1185 mib1185 added the enhancement New feature or request label Sep 12, 2021
@mib1185 mib1185 added this to the v1.2.0 milestone Sep 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants