-
-
Notifications
You must be signed in to change notification settings - Fork 29.9k
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
Use Platform enum #63577
Use Platform enum #63577
Conversation
@@ -82,7 +87,7 @@ def setup(hass: HomeAssistant, config: ConfigType) -> bool: | |||
|
|||
hass.data[DOMAIN] = {"channels": channels, "client": qvrpro} | |||
|
|||
load_platform(hass, CAMERA_DOMAIN, DOMAIN, {}, config) | |||
load_platform(hass, Platform.CAMERA, DOMAIN, {}, config) |
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.
Please keep this change on hold until #63571 is approved
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.
Care to share why @epenet?
As in, the current typing is already valid? Thus this is already valid?
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 have changed everywhere the PLATFORMS
constants (or pretty much everywhere).
But for now there are 100s of places where load_platform
and async_load_platform
are still using a string or the imported domain. I therefore felt that since this PR was mixing up the two (some changes to PLATFORM constants, and some changes to load_platform method it was better to postpone the method changes.
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.
Sorry, I'm not following that comment, this code seems correct. It passes a platform to the load platform method... all platform 🤷
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.
Do we consider that async_setup_component
allows a Platform
enum?
core/homeassistant/helpers/discovery.py
Line 162 in b3c2ebd
setup_success = await setup.async_setup_component(hass, component, hass_config) |
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.
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.
Both PRs have been merge. I think it's safe to merge this one now too.
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.
Looks much cleaner this way!
faeb715
to
aa20b9b
Compare
I wonder if we could we use it in the config schema also? (See |
Though about that too. Would you like to open a PR with an example? |
You can go ahead. Wont be able to create one before tomorrow. |
Proposed change
Successor to #63495 and similar.
Only use
Platform
enum for platform APIs.For context: #63495 (comment)
/CC @epenet: Those were the remaining changes form all my closed PRs. I do think though that your PR #63571 is the way forward.
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: