Various type hint improvements #37952
Conversation
Thanks to Martin Hjelmare
homeassistant/runner.py
Outdated
| "set_default_executor", | ||
| warn_use( | ||
| loop.set_default_executor, "sets default executor on the event loop" | ||
| ), |
There was a problem hiding this comment.
Why is this needed? Looks really ugly.
There was a problem hiding this comment.
Ugliness is subjective, but it's because of python/mypy#2427, better than # type: ignore.
There was a problem hiding this comment.
Well, guess betterness is subjective too. type ignore is a big gun, but easier to grep later and may actually be flagged automatically later as no longer needed.
Will have a look at this once the rest here has been cleared.
There was a problem hiding this comment.
setattr not triggering the same errors as assigning to a method might be fixed one day in mypy though 😉
There was a problem hiding this comment.
Yeah, it could be fixed either way in mypy, or perhaps there are several related things to fix. Anyway it's good to construct our workarounds in away that they start to trigger errors when they're no longer needed, so we get to clean them up as soon as possible. And at least we know unneeded type ignores result in that, so from that POV that instead of a setattr would be preferable after all -- and at least it's more prominent. Will have a look.
There was a problem hiding this comment.
Changed, passes locally, let's see if CI passes. Some headscratching here because the above discussed item already had a type ignore, not sure why I ended touching it in the first place.
| @@ -134,7 +134,7 @@ def __init__( | |||
| self, | |||
| hass: HomeAssistant, | |||
| script: "Script", | |||
| variables: Optional[Sequence], | |||
| variables: TemplateVarsType, | |||
There was a problem hiding this comment.
This should still have Optional ?
There was a problem hiding this comment.
Already defined as such,
core/homeassistant/helpers/typing.py
Line 16 in dbcd5f4
There was a problem hiding this comment.
Isn't that weird to have optional be part of the "main" type. I feel like optional should be context specific.
There was a problem hiding this comment.
Gut feeling says yes, but in helpers.template where the variables by that type end up being used it is truly always optional. OTOH if there are some places where passing some variables is actually a must (can't tell offhand), then it would be good to remove optionality from the type itself.
Proposed change
Various type hint improvements.
Type of change
Additional information
Checklist
black --fast 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.The integration reached or maintains the following Integration Quality Scale: