Skip to content

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

Merged
balloob merged 2 commits into
devfrom
default-entity-naming
Jul 6, 2023
Merged

Fix not including device_name in friendly name if it is None#95485
balloob merged 2 commits into
devfrom
default-entity-naming

Conversation

@jbouwh
Copy link
Copy Markdown
Contributor

@jbouwh jbouwh commented Jun 28, 2023

Breaking change

Not sure

Proposed change

If both device_entry.name_by_user or device_entry.name are None, then it should not be used to build the friendly name.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • I have followed the perfect PR recommendations
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

@jbouwh jbouwh requested a review from a team as a code owner June 28, 2023 21:09
@home-assistant home-assistant Bot added bugfix cla-signed core small-pr PRs with less than 30 lines. labels Jun 28, 2023
@jbouwh jbouwh added this to the 2023.7.0 milestone Jun 28, 2023
):
return name

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

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

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

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

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

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

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

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

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.

@jbouwh
Copy link
Copy Markdown
Contributor Author

jbouwh commented Jun 30, 2023

Thnx @balloob !

@jbouwh jbouwh closed this Jun 30, 2023
@github-actions github-actions Bot locked and limited conversation to collaborators Jul 1, 2023
@jbouwh jbouwh reopened this Jul 5, 2023
@jbouwh jbouwh marked this pull request as draft July 5, 2023 08:07
@jbouwh
Copy link
Copy Markdown
Contributor Author

jbouwh commented Jul 5, 2023

Reopening since the default device_name still can be None

@jbouwh jbouwh force-pushed the default-entity-naming branch from ebd9521 to d5a5a1c Compare July 5, 2023 08:14
@frenck frenck modified the milestones: 2023.7.0, 2023.7.1 Jul 5, 2023
@balloob balloob marked this pull request as ready for review July 6, 2023 15:13
@balloob balloob merged commit b9c7e7c into dev Jul 6, 2023
@balloob balloob deleted the default-entity-naming branch July 6, 2023 15:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants