Skip to content

Fix WUnderground names#12346

Merged
balloob merged 2 commits into
home-assistant:devfrom
OttoWinter:fix-wunderground-names
Feb 12, 2018
Merged

Fix WUnderground names#12346
balloob merged 2 commits into
home-assistant:devfrom
OttoWinter:fix-wunderground-names

Conversation

@OttoWinter
Copy link
Copy Markdown
Member

@OttoWinter OttoWinter commented Feb 12, 2018

Description:

With the changes made for the entity registry, WUnderground's (horrible) way of reporting friendly names to Home Assistant's core broke. Before, each WUnderground sensor would set the friendly name by setting the ATTR_FRIENDLY_NAME attribute on the sensor and the entity_id would be reported through the name property. This no longer works with the following code that creates the entity_id for each entity and no longer takes into account attributes[ATTR_FRIENDLY_NAME]:

https://github.com/home-assistant/home-assistant/blob/034eb9ae1aa33cec829393f409343919c036044c/homeassistant/helpers/entity_platform.py#L230-L233

This PR simply makes WUnderground use the name property for the friendly name and the entity_id attribute to set the entity_id.

Before:

screen shot 2018-02-12 at 17 42 19

After:

screen shot 2018-02-12 at 17 42 12

Related issue (if applicable): fixes #12282

Example entry for configuration.yaml (if applicable):

sensor:
  - platform: wunderground
    api_key: !secret wunderground_api_key
    monitored_conditions:
      - weather
      - weather_1d_metric
      - weather_1n_metric

Checklist:

  • The code change is tested and works locally.
  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • Tests have been added to verify that the new code works.

@OttoWinter
Copy link
Copy Markdown
Member Author

So I just went looking a bit at recent code changes to figure out how entity_id creation was done previously and when it "broke" for some components. Specifically, I looked a lot for ATTR_FRIENDLY_NAME being used in entity id creation - but without success. While I still believe the attribute ATTR_FRIENDLY_NAME and the property name of an Entity should always be the same (not like previous WUnderground), I currently have no idea how this even worked before.

@balloob
Copy link
Copy Markdown
Member

balloob commented Feb 12, 2018

I changed that logic here: https://github.com/home-assistant/home-assistant/pull/12237/files#diff-2f6e7e067d836cc94edba0d7d51d0241L227 . We should always have just looked at self.name instead of seeing if it was already overridden.

self._entity_picture = None
self._unit_of_measurement = self._cfg_expand("unit_of_measurement")
self.rest.request_feature(SENSOR_TYPES[condition].feature)
self.entity_id = async_generate_entity_id(
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.

You are not allowed to call async functions from a sync context.

This is not executed in an async context (because it's setup_platform instead of async_setup_platform).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good catch! Fixed.

Copy link
Copy Markdown
Member

@balloob balloob left a comment

Choose a reason for hiding this comment

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

fix async in sync context

@balloob balloob added this to the 0.63.2 milestone Feb 12, 2018
@balloob balloob merged commit 2c20269 into home-assistant:dev Feb 12, 2018
@rpitera
Copy link
Copy Markdown

rpitera commented Feb 13, 2018

Just a note; this fixed the sensors but missed the forecast entities. Restarted after 0.63.1, cleared browser cache and restarted browser. All forecast entities are still displaying this behavior. Let me know if you want me to open a new issue.

clipboard01

@OttoWinter
Copy link
Copy Markdown
Member Author

OttoWinter commented Feb 13, 2018

@rpitera Works for me :/ (Yes it's quite cold here in Austria 😄)

Question: Are you sure you've manually patched this into your install? Because this PR isn't in Home Assistant yet - though it will be in 0.63.2

screen shot 2018-02-13 at 17 27 04

@rpitera
Copy link
Copy Markdown

rpitera commented Feb 13, 2018

I checked my perms and they are correct; do I have new wunderground.py in the wrong location maybe? I have it in

/custom_components/components/sensor

I just restarted and cleared the cache again but still seeing the same thing. Only on the forecast sensors, everything else is fine.

balloob pushed a commit that referenced this pull request Feb 14, 2018
* 📝 Fix WUnderground names

* 👻 Fix using event loop callback
@balloob balloob mentioned this pull request Feb 14, 2018
@rpitera
Copy link
Copy Markdown

rpitera commented Feb 14, 2018

So just to follow up, 0.63.2 did fix the problem for me; I removed the test code prior to update and restart.

But I am still confused as to why the test code didn't work or whether I had it in the wrong location.

@OttoWinter OttoWinter deleted the fix-wunderground-names branch February 14, 2018 17:51
@amelchio
Copy link
Copy Markdown
Contributor

@rpitera It should be just custom_components/sensor/

@rpitera
Copy link
Copy Markdown

rpitera commented Feb 18, 2018

Thanks! It's been awhile since I tested anything and I forgot.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0.63 Weather Underground Friendly Names Lost

5 participants