Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
25 changes: 21 additions & 4 deletions homeassistant/components/sensor/wunderground.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,12 @@
import voluptuous as vol

from homeassistant.helpers.typing import HomeAssistantType, ConfigType
from homeassistant.components import sensor
from homeassistant.components.sensor import PLATFORM_SCHEMA, ENTITY_ID_FORMAT
from homeassistant.const import (
CONF_MONITORED_CONDITIONS, CONF_API_KEY, CONF_LATITUDE, CONF_LONGITUDE,
TEMP_FAHRENHEIT, TEMP_CELSIUS, LENGTH_INCHES, LENGTH_KILOMETERS,
LENGTH_MILES, LENGTH_FEET, ATTR_ATTRIBUTION)
LENGTH_MILES, LENGTH_FEET, ATTR_ATTRIBUTION, CONF_ENTITY_NAMESPACE)
from homeassistant.exceptions import PlatformNotReady
from homeassistant.helpers.entity import Entity, async_generate_entity_id
from homeassistant.helpers.aiohttp_client import async_get_clientsession
Expand Down Expand Up @@ -617,6 +618,8 @@ def _get_attributes(rest):
'CY', 'SN', 'JI', 'YI',
]

DEFAULT_ENTITY_NAMESPACE = 'pws'

PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend({

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 literally extending a platform schema here that includes entity namespace.

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.

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.

Hmm, there should be 🤔

vol.Required(CONF_API_KEY): cv.string,
vol.Optional(CONF_PWS_ID): cv.string,
Expand All @@ -627,22 +630,31 @@ def _get_attributes(rest):
'Latitude and longitude must exist together'): cv.longitude,
vol.Required(CONF_MONITORED_CONDITIONS):
vol.All(cv.ensure_list, vol.Length(min=1), [vol.In(SENSOR_TYPES)]),
vol.Optional(CONF_ENTITY_NAMESPACE,

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.

@OttoWinter OttoWinter Mar 17, 2018

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.

Yes, but we're generating our own entity ids here. So the namespace is never used in the entity component level - the entity namespace code is never executed in the entity component. we have to do it on our own here :/ (There might be another way though.)

default=DEFAULT_ENTITY_NAMESPACE): cv.string,
})

# Stores a list of entity ids we added in order to support multiple stations
# at once.
ADDED_ENTITY_IDS_KEY = 'wunderground_added_entity_ids'


@asyncio.coroutine
def async_setup_platform(hass: HomeAssistantType, config: ConfigType,
async_add_devices, discovery_info=None):
"""Set up the WUnderground sensor."""
hass.data.setdefault(ADDED_ENTITY_IDS_KEY, set())

latitude = config.get(CONF_LATITUDE, hass.config.latitude)
longitude = config.get(CONF_LONGITUDE, hass.config.longitude)
namespace = config.get(CONF_ENTITY_NAMESPACE)

rest = WUndergroundData(
hass, config.get(CONF_API_KEY), config.get(CONF_PWS_ID),
config.get(CONF_LANG), latitude, longitude)
sensors = []
for variable in config[CONF_MONITORED_CONDITIONS]:
sensors.append(WUndergroundSensor(hass, rest, variable))
sensors.append(WUndergroundSensor(hass, rest, variable, namespace))

yield from rest.async_update()
if not rest.data:
Expand All @@ -654,7 +666,8 @@ def async_setup_platform(hass: HomeAssistantType, config: ConfigType,
class WUndergroundSensor(Entity):
"""Implementing the WUnderground sensor."""

def __init__(self, hass: HomeAssistantType, rest, condition):
def __init__(self, hass: HomeAssistantType, rest, condition,
namespace: str):
"""Initialize the sensor."""
self.rest = rest
self._condition = condition
Expand All @@ -666,8 +679,12 @@ def __init__(self, hass: HomeAssistantType, rest, condition):
self._entity_picture = None
self._unit_of_measurement = self._cfg_expand("unit_of_measurement")
self.rest.request_feature(SENSOR_TYPES[condition].feature)
current_ids = set(hass.states.async_entity_ids(sensor.DOMAIN))

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.

Why would you generate this in every constructor call? Just do it once and pass it into the constructor

@OttoWinter OttoWinter Mar 17, 2018

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.

Hmm, true... that's inefficient indeed.

(on a side note: all entities (from all entity platforms) do not cache this: https://github.com/home-assistant/home-assistant/blob/4d3743f3f79a1127208ce885ae501991ec3e704e/homeassistant/helpers/entity_platform.py#L239-L240)

current_ids |= hass.data[ADDED_ENTITY_IDS_KEY]
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.

The real problem here in this component is that this entity is taking care of it's own entity ids. That is not the preferred approach. And guess what, because of this hack, we now need to add another hack to support entity namespace? Seems like a red flag to me…

@OttoWinter OttoWinter Mar 17, 2018

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.

It is a hack - this whole platform is very hackish :/

I just don't see any other way we can do custom names + constant "stable" entity ids at the same time. I mean we could take a look at disabling custom names to clean up the platform. Or we could even at some point take a look at transitioning this to the weather component.

If you want I can revert this. I will also make sure to wait for more feedback before merging these sorts of PRs that are a bit of a hack.

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.

I think that the real solution would be to do nothing with entity ids and just give it a unique id. Then people can rename it themselves.

For now let's keep it in, I guess? I don't really care that strongly.

ENTITY_ID_FORMAT, "pws_" + condition, hass=hass)
ENTITY_ID_FORMAT, "{} {}".format(namespace, condition),
current_ids=current_ids)
hass.data[ADDED_ENTITY_IDS_KEY].add(self.entity_id)

def _cfg_expand(self, what, default=None):
"""Parse and return sensor data."""
Expand Down
4 changes: 2 additions & 2 deletions homeassistant/helpers/entity.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
import functools as ft
from timeit import default_timer as timer

from typing import Optional, List
from typing import Optional, List, Iterable

from homeassistant.const import (
ATTR_ASSUMED_STATE, ATTR_FRIENDLY_NAME, ATTR_HIDDEN, ATTR_ICON,
Expand Down Expand Up @@ -42,7 +42,7 @@ def generate_entity_id(entity_id_format: str, name: Optional[str],

@callback
def async_generate_entity_id(entity_id_format: str, name: Optional[str],
current_ids: Optional[List[str]] = None,
current_ids: Optional[Iterable[str]] = None,
hass: Optional[HomeAssistant] = None) -> str:
"""Generate a unique entity ID based on given entity IDs or used IDs."""
if current_ids is None:
Expand Down
4 changes: 2 additions & 2 deletions homeassistant/util/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
from types import MappingProxyType
from unicodedata import normalize

from typing import Any, Optional, TypeVar, Callable, Sequence, KeysView, Union
from typing import Any, Optional, TypeVar, Callable, KeysView, Union, Iterable

from .dt import as_local, utcnow

Expand Down Expand Up @@ -72,7 +72,7 @@ def convert(value: T, to_type: Callable[[T], U],


def ensure_unique_string(preferred_string: str, current_strings:
Union[Sequence[str], KeysView[str]]) -> str:
Union[Iterable[str], KeysView[str]]) -> str:
"""Return a string that is not present in current_strings.

If preferred string exists will append _2, _3, ..
Expand Down
20 changes: 20 additions & 0 deletions tests/components/sensor/test_wunderground.py
Original file line number Diff line number Diff line change
Expand Up @@ -143,3 +143,23 @@ def test_invalid_data(hass, aioclient_mock):
for condition in VALID_CONFIG['monitored_conditions']:
state = hass.states.get('sensor.pws_' + condition)
assert state.state == STATE_UNKNOWN


async def test_entity_id_with_multiple_stations(hass, aioclient_mock):
"""Test not generating duplicate entity ids with multiple stations."""
aioclient_mock.get(URL, text=load_fixture('wunderground-valid.json'))

config = [
VALID_CONFIG,
{**VALID_CONFIG, 'entity_namespace': 'hi'}
]
await async_setup_component(hass, 'sensor', {'sensor': config})
await hass.async_block_till_done()

state = hass.states.get('sensor.pws_weather')
assert state is not None
assert state.state == 'Clear'

state = hass.states.get('sensor.hi_weather')
assert state is not None
assert state.state == 'Clear'