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 decorator type annotations [zwave_js] #104825
Improve decorator type annotations [zwave_js] #104825
Conversation
Hey there @home-assistant/z-wave, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
This seems good to me but typing is not my strong suit so I'd prefer @MartinHjelmare to review |
) -> None: | ||
"""Provide user specific data and store to function.""" | ||
entry, client, driver = await _async_get_entry( | ||
hass, connection, msg, msg[ENTRY_ID] | ||
) | ||
|
||
if not entry and not client and not driver: | ||
if not entry or not client or not driver: |
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.
We shouldn't change logic in a typing PR. If this is a bug it should be fixed separately.
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 change is only necessary to satisfy typing, it doesn't modify the logic.
_async_get_entry
will return either (None, None, None)
or (entry, client, driver)
. I've updated the annotation there as well to make that clear. After the tuple is split into three variables, type checkers evaluate each one separately. So an OR
-check is necessary here.
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. In the future I think we should consider raising an exception in _async_get_entry
instead of returning None
. That will make that function and its typing more clear.
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
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.
Thanks!
Proposed change
SSIA
Split up into different PRs to be able to wait for code owner approval.
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
..coveragerc
.To help with the load of incoming pull requests: