-
-
Notifications
You must be signed in to change notification settings - Fork 30.4k
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
improve handling of incorrect values in fyta integration #115134
improve handling of incorrect values in fyta integration #115134
Conversation
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.
Wouldn't it make more sense to just replace the value_fn in the places where the value is not one of the expected ones?
Current:
value_fn=lambda value: PLANT_STATUS[value],
value_fn=lambda value: PLANT_STATUS.get(value),
If value is then not in PLANT_STATUS
it will return None
which will render as unknown
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Oh and can you bump the dependency in a separate PR? |
I personally thought that it makes more sense to show the sensor as "unavailable", as - at least during the setup phase - there is not (yet) a correct value available. I could also live with the state being |
Sure. I thought as it is related to the same issue (and part of the issue is solved with the bump), it would make more sense in one PR... |
Nope! But in HA, unavailable is usually treated as "we could not reach the device" and unknown as "we received something, but we still don't know", so I think correct is more appropriate here. |
It has some benefits from making it a separate PR, you can see all the changes needed to work with the new lib in one place, they are usually reviewed way quicker, and now in this case, we might have feedback on the other code, causing the whole CI to run again (and this will probably just be 1 feedback cycle, but if this happens 4 times, it kinda clogs up the CI). |
Co-authored-by: Joost Lekkerkerker <joostlek@outlook.com>
done: #115143 |
* improve handling of incorrect values * Changes based on review comment * Apply suggestions from code review Co-authored-by: Joost Lekkerkerker <joostlek@outlook.com> * update value_fn * ruff --------- Co-authored-by: Joost Lekkerkerker <joostlek@outlook.com>
Proposed change
The FYTA server takes some time to process the data received from Beam sensors newly added to the system. While those new sensors already show up in the API, the reported values are not (yet) correct. With this PR additional tests are included to catch such incorrect values and set the entity to unavailable as long as no correct values are reported.
In fact, the PR covers the handling of possible wrong status values (a status value between 1 and 5 is expected).
Type of change
Additional information
Checklist
ruff format homeassistant tests
)If user exposed functionality or configuration variables are added/changed:
- [ ] Documentation added/updated for www.home-assistant.ioIf the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest
.- [ ] New or updated dependencies have been added torequirements_all.txt
.Updated by running
python3 -m script.gen_requirements_all
.~~- [ ] For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description. ~~
- [ ] Untested files have been added to.coveragerc
.To help with the load of incoming pull requests: