Enable strict typing checking for bluesound integration #123821
Conversation
home-assistant
bot
added
cla-signed
code-quality
integration: bluesound
small-pr
|
Hey there @thrawnarn, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
|
PR description is missing a link to the changelog, or at minimum a diff between library versions |
LouisChrist
force-pushed
the
bluesound_strict_typing
branch
from
924c61a to
f238a56
Compare
| @@ -239,15 +239,15 @@ def __init__( | |||
| self.port = port | |||
| self._polling_task: Task[None] | None = None # The actual polling task. | |||
| self._id = sync_status.id | |||
| self._last_status_update = None | |||
| self._last_status_update: datetime | None = None | |||
| self._sync_status = sync_status | |||
| self._status: Status | None = None | |||
| self._inputs: list[Input] = [] | |||
| self._presets: list[Preset] = [] | |||
| self._muted = False | |||
| self._master: BluesoundPlayer | None = None | |||
There was a problem hiding this comment.
Can we in followups also phase out the usage of master/slave naming and replace that with main/secondary?
There was a problem hiding this comment.
For sure. I have a refactor for the whole player grouping thing planned. There is one thing: It will be a breaking change. There is a custom attribute master visible to the user.
There was a problem hiding this comment.
What does that return? If we can deprecate attributes and turn them into entities (binary or normal sensors) if possible
There was a problem hiding this comment.
Its a boolean. But the group_members attribute from the media_player platform might enough to replace it.
| @@ -407,15 +399,15 @@ async def async_update_status(self): | |||
| ) | |||
| raise | |||
|
|
|||
| async def async_trigger_sync_on_all(self): | |||
| async def async_trigger_sync_on_all(self) -> None: | |||
| """Trigger sync status update on all devices.""" | |||
| _LOGGER.debug("Trigger sync status on all devices") | |||
|
|
|||
| for player in self.hass.data[DATA_BLUESOUND]: | |||
| await player.force_update_sync_status() | |||
|
|
|||
| @Throttle(SYNC_STATUS_INTERVAL) | |||
There was a problem hiding this comment.
It would also be nice to maybe find a way to phase out usage of throttle, since that function spawns a number of executor threads which isn't very efficient
There was a problem hiding this comment.
In a separate PR? This seems to be the wrong place the change.
There was a problem hiding this comment.
Oh definitely, just wanted to leave some remarks
There was a problem hiding this comment.
Are you sure about the spawning of executor threads? I had a look at it and it seems to skip calls if they are to soon or one is still running.
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Proposed change
This enables strict type checking and fixes all found errors. It also includes a new
pybluversion, because the old version did not have type checking itself and had some type errors.Changelog:
All changes v0.4.0...v1.0.0
Type 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.To help with the load of incoming pull requests: