Differentiate between device info types #95641
Conversation
| "default_model", | ||
| "default_name", | ||
| # Used by Fritz | ||
| "via_device", |
There was a problem hiding this comment.
I didn't expect this to be set to indicate which network device is used to connect a device to the network. I guess it fits, but other integrations don't do it.
balloob
force-pushed
the
define-device-type-info
branch
2 times, most recently
from
94aad19 to
03727f9
Compare
balloob
force-pushed
the
define-device-type-info
branch
2 times, most recently
from
d1ee589 to
34bfdb1
Compare
Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
balloob
force-pushed
the
define-device-type-info
branch
from
de833b8 to
c56794c
Compare
| self.logger.error( | ||
| "Ignoring device info without identifiers or connections: %s", | ||
| device_info, | ||
| ) | ||
| return None |
There was a problem hiding this comment.
It didn't raise in the old code either, but is there a good reason to not raise here? Why do we go ahead and add an entity which has invalid device info?
I'm not suggesting we change the behavior in this PR, but it's something to consider in a follow-up I think.
There was a problem hiding this comment.
I chose not to change that behavior as it would be breaking. It would probably be a small breaking change as I was pleasantly surprised the low amount of bad device info this PR found. If we decide to do this, we should do it in a follow-up PR.
| self.logger.error( | ||
| "Device info for %s needs to either describe a device, " | ||
| "link to existing device or provide extra information.", | ||
| device_info, | ||
| ) | ||
| return None |
There was a problem hiding this comment.
Same comment here, should this raise instead?
| self.logger.error( | ||
| "Ignoring device info with invalid configuration_url '%s'", | ||
| config_url, | ||
| ) | ||
| return None |
| @@ -10,4 +10,5 @@ def fritz_fixture() -> Mock: | |||
| with patch("homeassistant.components.fritzbox.Fritzhome") as fritz, patch( | |||
| "homeassistant.components.fritzbox.config_flow.Fritzhome" | |||
| ): | |||
| fritz.return_value.get_prefixed_host.return_value = "http://1.2.3.4" | |||
There was a problem hiding this comment.
Why is this test changed?
There was a problem hiding this comment.
The test used to return a Mock for this function, which resulted in passing Mock to configuration_url which triggered invalid device info, and device not to be created.
There was a problem hiding this comment.
I have slightly changed the behavior here. In the past, an invalid configuration URL would merely result in removal of the invalid configuration URL. With the updated logic, we will actually mark the whole device info as invalid and not create a device.
| @@ -114,6 +114,7 @@ def create_mock_client() -> Mock: | |||
| mock_client.instances = [ | |||
| {"friendly_name": "Test instance 1", "instance": 0, "running": True} | |||
| ] | |||
| mock_client.remote_url = f"http://{TEST_HOST}:{TEST_PORT_UI}" | |||
There was a problem hiding this comment.
Why is this test changed?
| @@ -19,6 +19,7 @@ def api_fixture(get_sensors_response): | |||
| """Define a fixture to return a mocked aiopurple API object.""" | |||
| return Mock( | |||
| async_check_api_key=AsyncMock(), | |||
| get_map_url=Mock(return_value="http://example.com"), | |||
There was a problem hiding this comment.
Why is this test changed?
Co-authored-by: Erik Montnemery <erik@montnemery.com>
* Differentiate between device info types * Update allowed fields * Update homeassistant/helpers/entity_platform.py Co-authored-by: Martin Hjelmare <marhje52@gmail.com> * Split up message in 2 lines * Use dict for device info types * Extract device info function and test error checking * Simplify parsing device info * move checks around * Simplify more * Move error checking around * Fix order * fallback config entry title to domain * Remove fallback for name to config entry domain * Ensure mocked configuration URLs are strings * one more test case * Apply suggestions from code review Co-authored-by: Erik Montnemery <erik@montnemery.com> --------- Co-authored-by: Martin Hjelmare <marhje52@gmail.com> Co-authored-by: Erik Montnemery <erik@montnemery.com>
Breaking change
Home Assistant is now differentiating between 3 different types of device info that can be included with an entity. It will reject device info values that include keys belonging to 2 different types.
Proposed change
This is a first step to make
device_infoproperty on an entity more strict. Eventual goal will be to know which config entry is the primary entry making a device.This first step is going to distinguish between 3 types of device info that we expect:
Requires:
Architecture discussion home-assistant/architecture#936
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.To help with the load of incoming pull requests: