Make FlowResult a generic type #111952
Make FlowResult a generic type #111952
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
|
homeassistant/auth/__init__.py
Outdated
| """Create a login flow.""" | ||
| auth_provider = self.auth_manager.get_auth_provider(*handler_key) | ||
| if not auth_provider: | ||
| raise KeyError(f"Unknown auth provider {handler_key}") | ||
| return await auth_provider.async_login_flow(context) | ||
|
|
||
| async def async_finish_flow( | ||
| self, flow: data_entry_flow.BaseFlowHandler, result: FlowResult | ||
| ) -> FlowResult: | ||
| self, flow: data_entry_flow.BaseFlowHandler, result: AuthFlowResult |
There was a problem hiding this comment.
| self, flow: data_entry_flow.BaseFlowHandler, result: AuthFlowResult | |
| self, flow: LoginFlow, result: AuthFlowResult |
Does this work? It would make the cast unnecessary.
There was a problem hiding this comment.
It doesn't work because:
homeassistant/auth/__init__.py:118: error: Argument 1 of "async_finish_flow" is incompatible with supertype "FlowManager"; supertype defines the argument type as "FlowHandler[AuthFlowResult, tuple[str, str]]" [override]
homeassistant/auth/__init__.py:118: note: This violates the Liskov substitution principle
homeassistant/auth/__init__.py:118: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#incompatible-overrides
I think mypy is being a bit unreasonable here though.
One option could be to provide the type of FlowHandler as a generic parameter to FlowManager.
There was a problem hiding this comment.
One option could be to provide the type of
FlowHandleras a generic parameter toFlowManager.
Wouldn't that also require higher-kind TypeVars as FlowHandler itself is generic?
I'd just leave it as is. Although nice, the current typing doesn't hurt and it's just one cast.
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
I missed that case, it should indeed only accept |
|
@emontnemery Could you rebase this one? |
emontnemery
force-pushed
the
flow-result-generic-type
branch
from
14316bb
to
6a12def
Compare
There was a problem hiding this comment.
This is coming together quite nicely 👍🏻
Just a few minor comments.
--
Regarding the TypeError. That looks like an issue with runtime typing and TypeVar defaults. More precisely one in typing_extensions as it doesn't overwrite the _check_generics function it ignore TypeVars with defaults when counting the generic arguments.
As we don't need runtime typing, it's easy enough to work around it. Just wrap the bound type in quotes here.
core/homeassistant/helpers/data_entry_flow.py
Lines 18 to 22 in 1daaffc
I'll see if I can open an issue in typing_extensions to fix that. Should be easy enough as the fix will also be required for 3.13. So it might already be implemented upstream in which case it only needs to be ported typing_extensions.
Edit:
| @@ -182,7 +182,7 @@ def __init__(self) -> None: | |||
|
|
|||
| @property | |||
| @abstractmethod | |||
| def flow_manager(self) -> FlowManager[ConfigFlowResult]: | |||
| def flow_manager(self) -> FlowManager[ConfigFlowResult, str]: | |||
There was a problem hiding this comment.
| def flow_manager(self) -> FlowManager[ConfigFlowResult, str]: | |
| def flow_manager(self) -> FlowManager[ConfigFlowResult]: |
str is the default type so it isn't necessary.
There was a problem hiding this comment.
I don't really like this. Can the generic type be passed by name, then it's clearer what is changed from the default?
There was a problem hiding this comment.
Unfortunately not. That would have required PEP-637 (Indexing with keyword arguments).
I mentioned it somewhere else already, this is also one of the reasons why TypeVars with defaults must be after ones without. Just so it's more clear what the type argument is referring to.
As it looks now, str is necessary after all. This is also a runtime use so the typing_extension bug hits here too. At the moment, it only works if either all or no type arguments are passed to the generic.
Proposed change
Add generic type
BaseFlowResultRationale:
Without this PR,
FlowResult.handleris typed asstr, although the handler in auth flows is atuple[str, str]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: