Simplify vesync init loading #135052
Simplify vesync init loading #135052
Conversation
|
Hey there @markperdue, @webdjoe, @TheGardenMonkey, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
| device_dict = await async_process_devices(hass, manager) | ||
|
|
||
| forward_setups = hass.config_entries.async_forward_entry_setups | ||
| # forward_setups = hass.config_entries.async_forward_entry_setups |
There was a problem hiding this comment.
Please don't have commented out code
| _LOGGER = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| async def async_process_devices( | ||
| async def async_generate_device_list( |
There was a problem hiding this comment.
imo this can move to __init__.py now
There was a problem hiding this comment.
In the future more items will be added to common.py such as is_humidifer, probably is_switch, is_outlet. Based on that would you still like it moved?
There was a problem hiding this comment.
If we have shared helper functions I would think they fit better in an helpers.py or utils.py.
I personally find common.py a very easy catch all for everything that doesn't fit
tests/components/vesync/test_init.py
Outdated
| with patch.object(hass.config_entries, "async_forward_entry_setups") as setups_mock: | ||
| assert await async_setup_entry(hass, config_entry) | ||
| # Assert platforms loaded | ||
| await hass.async_block_till_done() | ||
| assert setups_mock.call_count == 1 |
There was a problem hiding this comment.
Please don't patch out these internals. We should also rather use hass.config_entries.async_setup than calling async_setup_entry directly
There was a problem hiding this comment.
Could you point me to an example? I am not very familiar with tests in python or in general.
There was a problem hiding this comment.
There are quite a lot of examples, but you want to test that the integration is set up, and that's probably just a
assert await hass.config_entries.async_setup(entry.entry_id)
await hass.async_block_till_done()
assert entry.state is ConfigEntryState.LOADEDThere was a problem hiding this comment.
I think this is fixed. Let me know!
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Proposed change
Follow up from feedback here: #134425
Attempting to reduce the need for checks on devices in multiple places. From a experience standpoint mine is unchanged with this code change. This should allow adding a few other device types easier. We will also remove the requirement to map SKUs on the constants and shift to a feature focused check.
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.To help with the load of incoming pull requests: