Add generic classes BaseFlowHandler and BaseFlowManager #111814
Conversation
|
Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
|
Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
| @@ -241,6 +241,9 @@ class OperationNotAllowed(ConfigError): | |||
| } | |||
|
|
|||
|
|
|||
| ConfigFlowResult = FlowResult | |||
There was a problem hiding this comment.
When all config flows are migrated, this will be changed to a sub class of FlowResult which adds minor_version and version
There was a problem hiding this comment.
When all config flows are updated, this will be updated so it's only allowed to return ConfigFlowResult from ConfigFlow and OptionsFlow methods
| @@ -707,7 +717,7 @@ def _async_update_entry(self, data: dict[str, Any]) -> None: | |||
|
|
|||
| async def async_step_init( | |||
| self, user_input: dict[str, Any] | None = None | |||
| ) -> FlowResult: | |||
| ) -> ConfigFlowResult: | |||
There was a problem hiding this comment.
Do we need a different result for options flows?
There was a problem hiding this comment.
Yes, we should probably have that, but there are a handful of integrations which use the same base class for config and option flows and I couldn't work out a way to make the type checker accept it.
Should we give it another try before merging this PR or do you think it's OK to either let config and option flow share the return type or try to work it out in a follow-up?
There was a problem hiding this comment.
We can do it in a follow up.
homeassistant/data_entry_flow.py
Outdated
| self._handler_progress_index: dict[str, set[BaseFlowHandler]] = {} | ||
| self._init_data_process_index: dict[type, set[BaseFlowHandler]] = {} | ||
|
|
||
| _flow_result: Callable[..., _FlowResultT] |
There was a problem hiding this comment.
Where did Marc's comment go?
There was a problem hiding this comment.
| _flow_result: Callable[..., _FlowResultT] | |
| _flow_result: type[_FlowResultT] |
There was a problem hiding this comment.
And move it above the init method.
There was a problem hiding this comment.
Where did Marc's comment go?
type[_FlowResultT] would be ideal / the correct annotation, however mypy does appear to have an issue with it. Will need to investigate that one. That's why I deleted my initial comment (also thought I had only staged it in the first place - well guess I pressed the wrong button 😅)
Moving it above the __init__ method does still make sense though.
There was a problem hiding this comment.
I already tried the type version, but mypy does not like it like Marc suggests:
homeassistant/data_entry_flow.py:495: error: Cannot instantiate type "Type[FlowResult]" [misc]
We could add a comment explaining type would be preferred but is not allowed by mypy? Another alternative would be to type it as type and override the resulting errors. That's somewhat annoying though because child classes would then also need to silence errors if instantiating.
There was a problem hiding this comment.
FWIW, this is the only related mypy issue I could find when I first ran into the problem: python/mypy#12425, but that's about a nested type. In our case the type is from a typevar, not sure if it's the same problem or?
There was a problem hiding this comment.
type[X] and Callable[..., X] are basically equivalent in the return type. The only difference being that type[X] also checks the init args.
I think it's fine to leave Callable for now. Feel free to add a comment though that type would be preferred. We can adjust it if / when mypy is fixed.
There was a problem hiding this comment.
Found the mypy issue for it: python/mypy#11644
And here is the fix: python/mypy#16963. Should be in the release after the next one, i.e. 1.10.
Proposed change
This is the first of a set of PRs which splits up #105727 in managable pieces:
handleris a tuple of strings instead of a stringRationale:
FlowResultis currently a union of the results needed by all classes derived fromFlowHandler, and in some cases, derived classes knowingly violate the typing ofFlowResultType 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: