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
Refactor Tradfri switch device #26864
Conversation
|
||
TRADFRI_SWITCH_MANAGER = "Tradfri Switch Manager" | ||
TRADFRI_device_MANAGER = "Tradfri Switch Manager" |
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 understand what this is for. Does this have any functionality in Home Assistant's inner workings?
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.
No, it doesn't seem so. We can remove it.
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, I removed it
Pylint throws errors for |
According to Discord chats, there is some error causing pylint to fail. I’ll wait for the fix and rebase. |
self._available = False | ||
self.async_schedule_update_ha_state() | ||
_LOGGER.warning("Observation failed for %s", self._name, exc_info=exc) | ||
|
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.
Intentional to keep executing the rest of the function ?
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.
And if observation did fail, should you retry right away? Should we have some back off.
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'm afraid I'm not very familiar with how this part of the code works (looks like you and Lwis wrote it a long time ago.) I know as much that the _async_start_observe
looks more or less the same in all of the Tradfri device types, so we should be able to move it into the new base class.
However, I wouldn't know what I'm doing when making changes to _async_start_observe
. It would be a lot of guessing on my end. I'd be happy to just put in whatever you suggests. But that will require time for you to figure out the code. Perhaps a better idea to leave this method as is in the switch-file?
duration=0, | ||
) | ||
self.hass.async_create_task(self._api(cmd)) | ||
except PytradfriError as err: |
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.
What can raise here ? You are using create task so you're not waiting for the result.
@MartinHjelmare @balloob can this be merged on the basis of my comment above? The intent of this PR, along with a few subsequent ones, is just to remove code that is repeated across the Tradfri-files as well as stale constants. Once that is done, it will be much less work addressing the issues @balloob raises above. |
I think we can merge. @balloob ? |
Description:
As I was adding support for the Tradfri blinds, I realized there is a lot of repeated code in the different files of the Tradfri lib. This is the first step in refactoring the code.
Related issue (if applicable): relates to #26863
Pull request with documentation for home-assistant.io (if applicable): home-assistant/home-assistant.io#<home-assistant.io PR number goes here>
Example entry for
configuration.yaml
(if applicable):Checklist:
tox
. Your PR cannot be merged unless tests passIf user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
python3 -m script.hassfest
.requirements_all.txt
by runningpython3 -m script.gen_requirements_all
..coveragerc
.If the code does not interact with devices: