Fix Citybikes naming #12661

aronsky · 2018-02-25T15:10:24Z

Description:

The recent changes in Home Assistant core broke the naming scheme for CityBikes sensors. This PR fixes the naming.

Basically, up until now, Citybikes assigned each bike-sharing station a name based on the network and station IDs, to be used by HASS for generation of the entity ID of the station. The friendly name attribute was used to provide a nice, readable alternative to this ID.

However, following some recent changes (I believe PR #12237), the custom friendly name seems to be discarded by HASS, if the name attribute exists.

In over to overcome this, Citybikes now provides an entity_id property based on the previous template of network ID and station ID, while the name property is used to provide a friendly name, when available.

This change may be breaking, depending on user configuration, as the user-configurable name of the station cluster is not incorporated in the entity ID anymore.

Related issue (if applicable):
No issue was created, a direct fix is proposed here instead.

Checklist:

The code change is tested and works locally.
Local tests with tox run successfully. Your PR cannot be merged unless tests pass

MartinHjelmare · 2018-02-25T15:32:29Z

The problem is that the entity defines a friendly name attribute in the device state attributes. That's not the correct way. The entity should only define the friendly name in the name property.

OttoWinter · 2018-02-25T15:33:03Z

Basically the same as #12346; however the reason why this is happening is different from what you explained in the description of your PR if I understand correctly. Since the introduction of the entity registry attributes[ATTR_FRIENDLY_NAME] is no longer used for the displayed friendly name - instead only the name property is used (see #12346 (comment)).

Edit: Ah, @MartinHjelmare beat me to it :)

MartinHjelmare · 2018-02-25T15:46:09Z

Yes, but you added the better context.

aronsky · 2018-02-25T16:15:33Z

I see. No one commented on the use of ATTR_FRIENDLY_NAME before, when I first created the sensor, so I assumed that's how it was done. Anyway, with this change, the friendly name is reported via the name attribute, so should be in line with the current guidelines.

OttoWinter

You're still setting attributes[ATTR_FRIENDLY_NAME] here, I think you should remove that to remove potential confusion.

OttoWinter · 2018-02-25T16:20:50Z

homeassistant/components/sensor/citybikes.py

@@ -250,13 +251,19 @@ def state(self):
        """Return the state of the sensor."""
        return self._station_data.get(ATTR_FREE_BIKES, STATE_UNKNOWN)

+    @property
+    def entity_id(self):


While we're technically allowed to override the entity attribute, I don't think we should necessarily do that. I don't know the specifics about this platform, but maybe it could for some reason happen that two entities with the same entity_id are generated, thus breaking the second entity.

I think you should instead dynamically generate the entity_id through generate_entity_id. Then you'd also not need to slugify.

Thanks for the input! My initial idea was to use unique_id (which seems to be the perfect field to store this kind of information). However, since the changes in the way entity IDs are applied, the code in Home Assistant prefers to use the name property for entity ID generation, as opposed to unique_id (i.e., if the name property isn't None, it will use it to generate entity_id, even if unique_id exists). See here: https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/helpers/entity_platform.py#L197

I feel that for this platform (it shows the number of bikes available at different bike-sharing stations), an entity ID built from the bike sharing network ID/name and the network-specific station ID is more appropriate than the name of the station (which is usually an address). Maybe I'm wrong and not up to date on the philosophy behind entity IDs. However, the address is definitely more appropriate as a friendly name.

As for your concern about duplicate entity IDs, this shouldn't be a problem, as the entity_id property is rewritten by HASS anyway, as you can see here: https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/helpers/entity_platform.py#L222

aronsky · 2018-02-25T17:34:41Z

I'm open to changing the platform to use the station names as entity IDs.

The original idea was to generate the entity ID based on the bike sharing network name and the station ID while using the station name (usually, its address) as the friendly name. If people think that it's appropriate to use the name/address as the basis for the entity ID, I'm fine with that. I do insist on using the name/address as the friendly name that appears in the UI, as it is much more usable.

MartinHjelmare · 2018-02-25T17:34:59Z

homeassistant/components/sensor/citybikes.py

@@ -13,7 +13,8 @@
 import voluptuous as vol

 import homeassistant.helpers.config_validation as cv
-from homeassistant.components.sensor import PLATFORM_SCHEMA
+from homeassistant.components.sensor import (
+    PLATFORM_SCHEMA, DOMAIN as PARENT_DOMAIN)


Instead of importing as PARENT_DOMAIN, change the name of the existing DOMAIN constant in this module. You're not allowed to overwrite the component domain in the platform, in this case the sensor domain. Eg:

CITYBIKES_DATA = 'citybikes'

Fixed that.

MartinHjelmare · 2018-02-25T17:40:26Z

homeassistant/components/sensor/citybikes.py

+    @property
+    def entity_id(self):
+        """Return the entity ID."""
+        return "{}.{}_{}".format(PARENT_DOMAIN,


Remove the property and define an instance attribute self.entity_id in init instead. This isn't doing any I/O, correct?

And it is recommended to use generate_entity_id as @OttoWinter says.

Only entities that defines a unique ID get registered in the entity registry. So id conflict can still occur for all other entities.

For some reason, calling generate_entity_id gets stuck. I think it has something to do with the fact that the platform setup is async, but the call to generate_entity_id is performed in a constructor, which is not async. From looking at the code of other platforms and components, it seems that only platforms that aren't async use generate_entity_id, while others that need the functionality, simply create the ID on their own - which is what I've done here (after all, the logic for creating the ID is pretty straightforward, and shouldn't result in any duplicates).

Then you can just call async_generate_entity_id ;)

I tried that, that doesn't work either... :-/...
I'm supposed to yield from it, am I not? This is what I tried in the constructor:
self.entity_id = yield from async_generate_entity_id(ENTITY_ID_FORMAT, uid, hass=hass)

It results in an exception:

Traceback (most recent call last): File "/srv/homeassistant/lib/python3.6/site-packages/homeassistant/helpers/entity_platform.py", line 84, in async_setup SLOW_SETUP_MAX_WAIT, loop=hass.loop) File "/usr/local/lib/python3.6/asyncio/tasks.py", line 358, in wait_for return fut.result() File "/usr/local/lib/python3.6/asyncio/futures.py", line 245, in result raise self._exception File "/usr/local/lib/python3.6/asyncio/tasks.py", line 180, in _step result = coro.send(None) File "/srv/homeassistant/lib/python3.6/site-packages/homeassistant/components/sensor/citybikes.py", line 172, in async_setup_platform devices.append(CityBikesStation(hass, network, station_id, name)) TypeError: __init__() should return None, not 'generator'

And when using the normal version, as I mentioned before, it just hangs inside the execution of generate_entity_id...

OK, after more experimentations, tried calling async_generate_entity_id directly (without yield from), and it worked. I'm confused - I thought I'm supposed to await or to yield from any coroutines...

It's not a coroutine, it's a callback.

It seems that the naming is a bit confusing, in that case... Oh well.

A function that is named async_* in home assistant means that the function needs to run in the event loop. Coroutines are marked with the @asyncio.coroutine annotation or defined with the python 3.5 keyword async def.

I see. Thanks!

Entity ID generation now includes the base name (again), resulting in preservation of the entity ID format (the change is non-breaking)

MartinHjelmare

Thanks! Can be merged when build passes.

aronsky · 2018-02-27T15:36:18Z

@MartinHjelmare sorry for the confusion. As I mentioned in one of the comments, the change ended up not being breaking after all, so no need to mark it as breaking in release notes.

MartinHjelmare · 2018-02-27T16:00:34Z

Great!

Fix Citybikes naming.

8b12c30

homeassistant added cla-signed platform: sensor.citybikes small-pr PRs with less than 30 lines. labels Feb 25, 2018

OttoWinter reviewed Feb 25, 2018

View reviewed changes

MartinHjelmare requested changes Feb 25, 2018

View reviewed changes

Semantic fixes upon requests.

4bdaccc

Entity ID generation now includes the base name (again), resulting in preservation of the entity ID format (the change is non-breaking)

homeassistant added the cla-signed label Feb 26, 2018

Use async_generate_entity_id.

c6fbbcb

MartinHjelmare added the breaking-change label Feb 27, 2018

MartinHjelmare approved these changes Feb 27, 2018

View reviewed changes

MartinHjelmare changed the title ~~Fix Citybikes naming.~~ Fix Citybikes naming Feb 27, 2018

MartinHjelmare merged commit be64422 into home-assistant:dev Feb 27, 2018

MartinHjelmare removed the breaking-change label Feb 27, 2018

balloob mentioned this pull request Mar 9, 2018

0.65 #12995

Merged

home-assistant locked and limited conversation to collaborators May 29, 2018

ghost added integration: citybikes and removed platform: sensor.citybikes labels Mar 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Citybikes naming #12661

Fix Citybikes naming #12661

aronsky commented Feb 25, 2018 •

edited by MartinHjelmare

MartinHjelmare commented Feb 25, 2018

OttoWinter commented Feb 25, 2018 •

edited

MartinHjelmare commented Feb 25, 2018

aronsky commented Feb 25, 2018

OttoWinter left a comment

OttoWinter Feb 25, 2018

aronsky Feb 25, 2018

aronsky commented Feb 25, 2018

MartinHjelmare Feb 25, 2018

aronsky Feb 26, 2018

MartinHjelmare Feb 25, 2018

MartinHjelmare Feb 25, 2018

aronsky Feb 26, 2018

OttoWinter Feb 26, 2018

aronsky Feb 27, 2018

aronsky Feb 27, 2018

MartinHjelmare Feb 27, 2018

aronsky Feb 27, 2018

MartinHjelmare Feb 27, 2018

aronsky Feb 27, 2018

MartinHjelmare left a comment

aronsky commented Feb 27, 2018

MartinHjelmare commented Feb 27, 2018

Fix Citybikes naming #12661

Fix Citybikes naming #12661

Conversation

aronsky commented Feb 25, 2018 • edited by MartinHjelmare

Description:

Checklist:

MartinHjelmare commented Feb 25, 2018

OttoWinter commented Feb 25, 2018 • edited

MartinHjelmare commented Feb 25, 2018

aronsky commented Feb 25, 2018

OttoWinter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aronsky commented Feb 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartinHjelmare left a comment

Choose a reason for hiding this comment

aronsky commented Feb 27, 2018

MartinHjelmare commented Feb 27, 2018

aronsky commented Feb 25, 2018 •

edited by MartinHjelmare

OttoWinter commented Feb 25, 2018 •

edited