Explicitly allow Platform enum in discovery helper #63571
Conversation
epenet
requested review from
michaelarnauts,
OverloadUT and
a team
as code owners
|
Hey there @jeroenterheerdt, mind taking a look at this pull request as it has been labeled with an integration ( |
|
Hey there @flacjacket, mind taking a look at this pull request as it has been labeled with an integration ( |
|
Hey there @OverloadUT, mind taking a look at this pull request as it has been labeled with an integration ( |
|
Hey there @michaelarnauts, mind taking a look at this pull request as it has been labeled with an integration ( |
This reverts commit bfff08f.
|
I have renamed the arguments. And I have kept just a single component (amcrest) to show how it would be used. |
homeassistant/helpers/discovery.py
Outdated
| if component not in hass.config.components: | ||
| setup_success = await setup.async_setup_component(hass, component, hass_config) | ||
| # Ensure that the underlying platform domain is setup | ||
| platform_domain = platform.value if isinstance(platform, Platform) else platform |
There was a problem hiding this comment.
We could add a frame.report here if we want to encourage people to use the Platform enum.
For example "if platform is a string and platform could be in Platform then report the frame"
There was a problem hiding this comment.
but since Platform is a helper and not a definitive list maybe it is not necessary to report it
There was a problem hiding this comment.
I think the better long term option would be to at some point update the type again and only allow Platform not str anymore. For now it's perfectly fine to pass a str. As I mentioned, they behave the same.
epenet
changed the title
|
cc @scop as you were heavily involved in the StrEnum |
There was a problem hiding this comment.
Similar comments to #63581 (review)
As for requiring Platform enum (with typing), at some point this might be an option. Don't know what others think about it though. I seem to remember that Paulus once said he is fine with passing a string. Not sure that's still current. home-assistant/architecture#663 (comment)
homeassistant/helpers/discovery.py
Outdated
| if component not in hass.config.components: | ||
| setup_success = await setup.async_setup_component(hass, component, hass_config) | ||
| # Ensure that the underlying platform domain is setup | ||
| platform_domain = platform.value if isinstance(platform, Platform) else platform |
There was a problem hiding this comment.
I think the better long term option would be to at some point update the type again and only allow Platform not str anymore. For now it's perfectly fine to pass a str. As I mentioned, they behave the same.
emontnemery
changed the title
Proposed change
Add Platform to type hint in
homeassistant/helpers/discovery.pyAdjust a few components accordingly (will need a follow-up PR to adjust all components but they are here as a sample)
cc @frenck / @cdce8p / @MartinHjelmare
This is a follow-up to the discussion in #63495 (comment)
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:
To help with the load of incoming pull requests: