Add Missing None return type in Vesync#161597
Conversation
|
Hey there @markperdue, @webdjoe, @TheGardenMonkey, @iprak, @SapuSeven, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
|
|
||
| @property | ||
| def current_humidity(self) -> int: | ||
| def current_humidity(self) -> int | None: |
There was a problem hiding this comment.
what does it mean if those two return None? Should the entity be unavailable instead?
There was a problem hiding this comment.
This seems fine? Returning None should cause the entity to assume unknown state, becoming greyed out in the frontend, similar as to when it’s unavailable.
The typing in the base Humidifier class already allows for this, see here.
There was a problem hiding this comment.
This seems fine?
Technically yes. We were just returning unknown too often in the past, instead of making the entity unavailable, that's why I'm usually asking when we are returning None
There was a problem hiding this comment.
This was done for mypy reasons. The variable the library starts as None until first API call. After that I wouldn't expect it to be None. If an update fails the device will go unavailable.
There was a problem hiding this comment.
then usually we tend to add
if TYPE_CHECKING:
assert ...so it's clear it won't return unkown
There was a problem hiding this comment.
Thank you! I love learning these tricks. Appreciated. Ready for review with the above included.
Proposed change
Return type cleanup to reduce size of #160573
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: