-
-
Notifications
You must be signed in to change notification settings - Fork 31.6k
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
Set Scrape sensor unavailable when errors #134143
base: dev
Are you sure you want to change the base?
Conversation
Hey there @fabaff, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
@@ -172,6 +172,7 @@ def _extract_value(self) -> Any: | |||
"""Parse the html extraction in the executor.""" | |||
raw_data = self.coordinator.data | |||
value: str | list[str] | None | |||
self._attr_available = True |
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.
This looks ok but my instinct says we should be using a temporary variable then assigning to the instance variable at the end, to avoid any possibility of having a misleading intermediate state.
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 you actually mean here why I should set another variable within the same function and why that would be better?
Or do you mean I should only write it in _async_update_from_rest_data
?
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.
My understanding is that setting self._attr_available = True
at the start means it will be toggled between True/False every time it is updated, even if the eventual value is False
.
So if there is threaded or asynchronous access to self._attr_available
part-way through, or if an unexpected exception gets raised, it would be read as an incorrect value.
It is possible that the HA architecture/scheduling prevents this though.
Something similar happens in #133336 which is what made me think of 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.
Well, you're not wrong but it's a bit theoretical as we write the state manually in the very end of the update process.
But I'll make an update (because I have also seen another "problem" which I will fix with this)
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Proposed change
Sets the Scrape sensor unavailable instead of unknown when there are errors
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: