Add get_torrents service to qBittorrent integration #106501
Conversation
This brings the QBittorrent integration to be more in line with the Transmission integration. It updates how the integration is written, along with adding sensors for Active Torrents, Inactive Torrents, Paused Torrents, Total Torrents, Seeding Torrents, Started Torrents.
This adds a service with a response to be able to get the list of torrents within qBittorrent
|
Hey there @geoffreylagaisse, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
|
@engrbm87 can you also take a look at this? Since I also promoted the use of a service call for transmission, maybe it's an idea to use a similar return schema so it's easier for custom card creators to create a card that works for both integrations. |
Sure, I will work on something similar for transmission. But I would like to know how the response can be displayed on the frontend (like a markdown card). |
I built a custom card that does that for qBittorrent with this new service. I can upload it to github later today |
Will this card work for any service response or is it specific for qBittorent. It would be hard to build a separate card for each integration. |
I mean the card is specifically built for qBittorrent in this case, but its using the standard HA way of calling a service with a response. So the knowledge gained from me building it can be used elsewhere. I built it off of https://github.com/amaximus/transmission-card, but customized it for qbittorrent and its new service call |
|
I'm slowly playing with the idea of "standardised service calls". We of course have them for our entity platforms like "climate.set_hvac" or "calendar.get_events". But I think it would benefit some types of integrations as well to have a standard service call. For example public transport integrations or download integrations. And currently there are no plans afaik to support something like that in core itself, but I'll fish around checking what others think of it and maybe we can implement it. This doesn't have to hold us back into making 2 "custom" services return in the same data structure |
|
And the idea behind it is that we don't create a complete new entity platform for public transport or download integrations, but just keep using these mostly sensor based integrations |
finder39
changed the title
|
The requested changes have been made |
| DOMAIN, | ||
| SERVICE_GET_TORRENTS, | ||
| handle_get_torrents, | ||
| supports_response=SupportsResponse.ONLY, |
There was a problem hiding this comment.
I'm missing a service validation schema for the services.
| @@ -7,6 +7,13 @@ | |||
| DEFAULT_NAME = "qBittorrent" | |||
| DEFAULT_URL = "http://127.0.0.1:8080" | |||
|
|
|||
| STATE_ATTR_TORRENTS = "torrents" | |||
| STATE_ATTR_ALL_TORRENTS = "all_torrents" | |||
There was a problem hiding this comment.
These are not state attributes. Please rename the constants.
| STATE_UP_DOWN = "up_down" | ||
| STATE_SEEDING = "seeding" | ||
| STATE_DOWNLOADING = "downloading" | ||
|
|
||
| SERVICE_GET_TORRENTS = "get_torrents" |
There was a problem hiding this comment.
We don't need constants for strings that are only used once.
|
|
||
| def __init__(self, hass: HomeAssistant, client: Client) -> None: | ||
| """Initialize coordinator.""" | ||
| self.client = client | ||
| # self.main_data: dict[str, int] = {} |
There was a problem hiding this comment.
Commented code. Please remove it.
| self.total_torrents: dict[str, int] = {} | ||
| self.active_torrents: dict[str, int] = {} | ||
| self.inactive_torrents: dict[str, int] = {} | ||
| self.paused_torrents: dict[str, int] = {} | ||
| self.seeding_torrents: dict[str, int] = {} | ||
| self.started_torrents: dict[str, int] = {} |
There was a problem hiding this comment.
These instance attributes are never used. Please remove them.
| try: | ||
| return await self.hass.async_add_executor_job(self.client.sync_main_data) | ||
| except LoginRequired as exc: | ||
| raise ConfigEntryError("Invalid authentication") from exc | ||
| raise HomeAssistantError(str(exc)) from exc |
There was a problem hiding this comment.
Why did we change the exception in this PR? That looks unrelated to the new services.
| @@ -10,3 +13,48 @@ def setup_client(url: str, username: str, password: str, verify_ssl: bool) -> Cl | |||
| # Get an arbitrary attribute to test if connection succeeds | |||
| client.get_alternative_speed_status() | |||
| return client | |||
|
|
|||
|
|
|||
| def seconds_to_hhmmss(seconds) -> str: | |||
There was a problem hiding this comment.
The parameter is missing a type annotation. Please type the whole signature when adding type annotations.
| "fields": { | ||
| "device_id": { | ||
| "name": "[%key:common::config_flow::data::device%]", | ||
| "description": "Which service to grab the list from" |
There was a problem hiding this comment.
"grab the list" is too colloquial English and not correctly explained what it's referring to.
Breaking change
Proposed change
This adds a
get_torrent()service to the qBittorrent inragrationType of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all..coveragerc.To help with the load of incoming pull requests: