Skip to content
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

Migrate Environment Canada to new entity naming style #75024

Merged
merged 5 commits into from
Jul 12, 2022
Merged

Migrate Environment Canada to new entity naming style #75024

merged 5 commits into from
Jul 12, 2022

Conversation

gwww
Copy link
Contributor

@gwww gwww commented Jul 11, 2022

Proposed change

Migrate Environment Canada to new entity naming style.

This may not be correct as the names all have the weather station as part of the name. For example "Ottawa Radar". Is the right thing to add device_info (for a service) that has a name of the weather station and then remove the station from all of the entities? An example of the code here: https://github.com/home-assistant/core/blob/dev/homeassistant/components/environment_canada/sensor.py#L295

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
  • 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.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

To help with the load of incoming pull requests:

@probot-home-assistant
Copy link

Hey there @michaeldavie, mind taking a look at this pull request as it has been labeled with an integration (environment_canada) you are listed as a code owner for? Thanks!
(message by CodeOwnersMention)

@frenck
Copy link
Member

frenck commented Jul 12, 2022

I think adding device information is generally a good thing to do! It unlocks device automations as well (Don't forget to set device type to "service").

@gwww
Copy link
Contributor Author

gwww commented Jul 12, 2022

I’ve already coded the device info. I’ll submit a separate PR. Thanks!!

@frenck
Copy link
Member

frenck commented Jul 12, 2022

I’ve already coded the device info. I’ll submit a separate PR. Thanks!!

I would add it into this one, as it requires name changes as well?

@gwww
Copy link
Contributor Author

gwww commented Jul 12, 2022

I'm not clear on what is the right thing to do with the entity names. Should I pull the site name out of the name? So something such as Ottawa Radar (camera.ottawa_radar) becomes Radar (camera.radar) and the Ottawa bit is part of the device info?

@frenck
Copy link
Member

frenck commented Jul 12, 2022

Sounds like Ottawa is the device, with its entities.

@gwww
Copy link
Contributor Author

gwww commented Jul 12, 2022

I guess what I still don't understand about the entity name change exercise is what happens when I have Ottawa Radar and Amsterdam Radar and they get renamed to be just Radar. Is that the intention of this exercise and entities get distinguished because of the device they are associated with? I read the blog post multiple times trying to understand and couldn't quite figure it out.

Edit: I'll let the code speak. I push the changes I think are in line with what the entity name change is trying to accomplish.

@frenck
Copy link
Member

frenck commented Jul 12, 2022

Let's say you have a Z-Wave multi-sensor. It provides multiple data points (entities in the Home Assistant world).

So, we have a device called, for example, "Multi-sensor" and two entities called "Temperature" & "Humidity".

When the new naming style is enabled, Home Assistant will create the two following entities (and combines the device name and entity name for the entity ID):

  • sensor.multi_sensor_temperature
  • sensor.multi_sensor_humidity

Depending on which place the entities are shown, Home Assistant will show the entity differently.

For example: If one is looking at the Device page, the title of that page would be "Multi-sensor" and show entities with just their own names: The "Temperature" & "Humidity entities. The device page already has context of the device you are looking at.

(Although this already works on the device page in the old naming style, it currently relies on search/replacement of strings on the device page, which is kinda nasty :S)

IN other places, Home Assistant adds context when needed (at this time, as usual, it shows the the combination of the device name and the entity name: "Multi-sensor Temperature" & "Multi-sensor Humidity".

For the future, we can change/adjust the view to show less or more context when needed (e.g., entity selectors could show the device name + area as secondary line info).

Right now, our goal is to untangle the "merged" device names into the entity names, resulting in each device, entity, area having their own names; Allowing Home Assistant to display them correctly depending on the context.

@gwww
Copy link
Contributor Author

gwww commented Jul 12, 2022

Thanks, that was very helpful. I now understand the intent! My current PR matches that intent, I believe.

Adding your reply to the blog post might help others.

Co-authored-by: Franck Nijhof <frenck@frenck.nl>
Copy link
Member

@frenck frenck left a comment

Choose a reason for hiding this comment

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

Thanks, @gwww 👍

@frenck frenck merged commit 96ecbe4 into home-assistant:dev Jul 12, 2022
@gwww gwww deleted the ec_entity_name branch July 12, 2022 18:55
@github-actions github-actions bot locked and limited conversation to collaborators Jul 13, 2022
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.

4 participants