Skip to content

Fix not including device_name in friendly name if it is None #95485

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.

Sign up for GitHub

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

Merged
balloob merged 2 commits into from
Jul 6, 2023
Merged

Fix not including device_name in friendly name if it is None #95485

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d5a5a1c
Omit device_name in friendly name if it is None
jbouwh Jun 28, 2023
b0a7cb6
Fix test
jbouwh Jul 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions homeassistant/helpers/entity.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -772,9 +772,10 @@ def _friendly_name_internal(self) -> str | None:
):
return name

device_name = device_entry.name_by_user or device_entry.name
Copy link
Copy Markdown
Member

@balloob balloob Jun 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes me wonder if we should even allow a device to have no name. Should we make up a random name in the device registry if name is empty?

Copy link
Copy Markdown
Contributor Author

@jbouwh jbouwh Jun 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure about that, it is that with mqtt we have some tests and examples where no device name is set, when then a default entity name is assigned we get a None prefix to the friendly name, and none_ is added to the 2nd part of the entity id. @emontnemery thinks this is a bug. In this case I would just omit the device part as it does not make sense to add it.

Copy link
Copy Markdown
Member

@balloob balloob Jun 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we reject the configuration instead? if you want to have your entity name be based on a device but there is no device name, it doesn't work ?

Copy link
Copy Markdown
Contributor Author

@jbouwh jbouwh Jun 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does work, you get an unnamed device. Now allowing this would be breaking, as a device name is optional.

Copy link
Copy Markdown
Member

@balloob balloob Jun 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, but shouldn't we phase in that it's required if has_entity_name is set to True?

Copy link
Copy Markdown
Contributor Author

@jbouwh jbouwh Jun 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds reasonable to me. If if for MQTT items we do not want to bother the user, we could set has_entity_name to False when no device name is set and make a device_name required when has_entity_name is set to True.

Copy link
Copy Markdown
Contributor Author

@jbouwh jbouwh Jun 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting has_entity_name to False if no device name is set would be a good alternative:
https://github.com/home-assistant/core/pull/95159/files#r1247719960

Copy link
Copy Markdown
Contributor Author

@jbouwh jbouwh Jun 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A case where this will not work is when we want to follow the name of the device_class. In that case has_entity_name must be set to True.

if self.use_device_name:
return device_entry.name_by_user or device_entry.name
return f"{device_entry.name_by_user or device_entry.name} {name}"
return device_name
return f"{device_name} {name}" if device_name else name

@callback
def _async_write_ha_state(self) -> None:
Expand Down
23 changes: 16 additions & 7 deletions tests/helpers/test_entity.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
from homeassistant.core import Context, HomeAssistant, HomeAssistantError
from homeassistant.helpers import device_registry as dr, entity, entity_registry as er
from homeassistant.helpers.entity_component import async_update_entity
from homeassistant.helpers.typing import UNDEFINED
from homeassistant.helpers.typing import UNDEFINED, UndefinedType

from tests.common import (
MockConfigEntry,
Expand Down Expand Up @@ -989,19 +989,28 @@ async def async_setup_entry(hass, config_entry, async_add_entities):


@pytest.mark.parametrize(
("has_entity_name", "entity_name", "expected_friendly_name", "warn_implicit_name"),
(
(False, "Entity Blu", "Entity Blu", False),
(False, None, None, False),
(True, "Entity Blu", "Device Bla Entity Blu", False),
(True, None, "Device Bla", False),
"has_entity_name",
"entity_name",
"device_name",
"expected_friendly_name",
"warn_implicit_name",
),
(
(False, "Entity Blu", "Device Bla", "Entity Blu", False),
(False, None, "Device Bla", None, False),
(True, "Entity Blu", "Device Bla", "Device Bla Entity Blu", False),
(True, None, "Device Bla", "Device Bla", False),
(True, "Entity Blu", UNDEFINED, "Entity Blu", False),
(True, "Entity Blu", None, "Mock Title Entity Blu", False),
),
)
async def test_friendly_name_attr(
hass: HomeAssistant,
caplog: pytest.LogCaptureFixture,
has_entity_name: bool,
entity_name: str | None,
device_name: str | None | UndefinedType,
expected_friendly_name: str | None,
warn_implicit_name: bool,
) -> None:
Expand All @@ -1012,7 +1021,7 @@ async def test_friendly_name_attr(
device_info={
"identifiers": {("hue", "1234")},
"connections": {(dr.CONNECTION_NETWORK_MAC, "abcd")},
"name": "Device Bla",
"name": device_name,
},
)
ent._attr_has_entity_name = has_entity_name
Expand Down