Skip to content
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

Merged

Conversation

dontinelli
Copy link
Contributor

@dontinelli dontinelli commented Apr 7, 2024

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

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • I have followed the perfect PR recommendations
  • The code has been formatted using Ruff (ruff format homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

- [ ] Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
    - [ ] New or updated dependencies have been added to requirements_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:

@dontinelli dontinelli changed the title improve handling of incorrect values improve handling of incorrect values in fyta integration Apr 7, 2024
Copy link
Member

@joostlek joostlek left a 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

@home-assistant home-assistant bot marked this pull request as draft April 7, 2024 18:11
@home-assistant
Copy link

home-assistant bot commented Apr 7, 2024

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@joostlek
Copy link
Member

joostlek commented Apr 7, 2024

Oh and can you bump the dependency in a separate PR?

@dontinelli
Copy link
Contributor Author

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

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 unknown. Is there a difference how it would be handled in the statistics?

@dontinelli
Copy link
Contributor Author

Oh and can you bump the dependency in a separate PR?

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...

@joostlek
Copy link
Member

joostlek commented Apr 7, 2024

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.

@joostlek
Copy link
Member

joostlek commented Apr 7, 2024

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).

dontinelli and others added 3 commits April 7, 2024 20:42
Co-authored-by: Joost Lekkerkerker <joostlek@outlook.com>
@joostlek joostlek added this to the 2024.4.2 milestone Apr 7, 2024
@dontinelli dontinelli marked this pull request as ready for review April 7, 2024 18:52
@home-assistant home-assistant bot requested a review from joostlek April 7, 2024 18:52
@dontinelli
Copy link
Contributor Author

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).

done: #115143

@joostlek joostlek merged commit 80ec9d4 into home-assistant:dev Apr 7, 2024
24 checks passed
@dontinelli dontinelli deleted the dontinelli-improve-data-handling branch April 7, 2024 19:14
frenck pushed a commit that referenced this pull request Apr 8, 2024
* 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>
@frenck frenck mentioned this pull request Apr 8, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Apr 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FYTA Error: Task exception was never retrieved - PLANT_STATUS[value],
3 participants