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

NSW Rural Fire Service platform #16802

Merged
merged 14 commits into from Oct 14, 2018

Conversation

Projects
None yet
5 participants
@exxamalte
Contributor

exxamalte commented Sep 23, 2018

Description:

This is a new platform in the geo location component. It retrieves information about bushfires from the NSW Rural Fire Service. The platform supports two filters: The common radius filter which lets users define a radius around their home so that only events occurring within that radius are included. Also, users can select one or more categories ('Emergency Warning', 'Watch and Act', 'Advice', 'Not Applicable') that they are interested in only.

Related issue (if applicable): evolving #15953

Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#6353

Example entry for configuration.yaml (if applicable):

geo_location:
  - platform: nsw_rural_fire_service_feed
    radius: 25
    categories:
      - 'Emergency Warning'
      - 'Watch and Act'

Checklist:

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

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

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

  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • Tests have been added to verify that the new code works.
@frenck

This comment has been minimized.

Member

frenck commented Sep 23, 2018

I was unable to find a related documentation PR on the docs repository. Added docs-missing label to this PR.

@exxamalte

This comment has been minimized.

Contributor

exxamalte commented Sep 23, 2018

Documentation is in progress and follows soon.

@exxamalte exxamalte referenced this pull request Sep 25, 2018

Merged

NSW Rural Fire Service platform #6353

2 of 2 tasks complete

@frenck frenck removed the docs-missing label Sep 30, 2018

@MartinHjelmare

Looks good! Two minor comments.

from homeassistant.components.geo_location import GeoLocationEvent
from homeassistant.const import CONF_RADIUS, CONF_SCAN_INTERVAL, \
EVENT_HOMEASSISTANT_START, ATTR_ATTRIBUTION
from homeassistant.components.geo_location import PLATFORM_SCHEMA

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Sep 30, 2018

Member

Group imports.

This comment has been minimized.

@exxamalte

exxamalte Sep 30, 2018

Contributor

OK

state = self.hass.states.get("geo_location.title_1")
self.assertIsNotNone(state)
assert state.name == "Title 1"
print(state.attributes)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Sep 30, 2018

Member

Stale debug print.

This comment has been minimized.

@exxamalte

exxamalte Sep 30, 2018

Contributor

Oops...

self._scan_interval = scan_interval
self._feed_entries = []
self._managed_entities = []
hass.bus.listen_once(

This comment has been minimized.

@balloob

balloob Oct 1, 2018

Member

Don't attach listeners in constructors. Instead do that in aync_added_to_hass callback

This comment has been minimized.

@exxamalte

exxamalte Oct 1, 2018

Contributor

OK

This comment has been minimized.

@exxamalte

exxamalte Oct 1, 2018

Contributor

Sorry, I just saw that async_added_to_hass only seems to work on sub-classes of Entity which is not the case here. I could move the code into the setup_platform method.

self._managed_entities = []
hass.bus.listen_once(
EVENT_HOMEASSISTANT_START, lambda _: self._update())
self._init_regular_updates()

This comment has been minimized.

@balloob

balloob Oct 1, 2018

Member

We should do this at HA start event callback

This comment has been minimized.

@exxamalte

exxamalte Oct 1, 2018

Contributor

OK, moving into async_added_to_hass, too

This comment has been minimized.

@exxamalte

exxamalte Oct 1, 2018

Contributor

Same as above - going to move into call from setup_platform instead.

if remove_entry:
feed_entries.remove(remove_entry)
remove_entry = None
for entry in feed_entries:

This comment has been minimized.

@balloob

balloob Oct 1, 2018

Member

Nested for loops are not good. Let's convert the feed entries into a look up table:

entries_lookup = {entry.external_id: entry for entry in feed_entries}

This comment has been minimized.

@exxamalte

exxamalte Oct 1, 2018

Contributor

Thanks. That had some good flow-on effects for simplifying the code.

entries_lookup = {entry.external_id: entry for entry in feed_entries}
for entity in managed_entities:
if entity.external_id in entries_lookup:
entry = entries_lookup.get(entity.external_id)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 2, 2018

Member

Use dict.pop instead since we want to remove the item from the dict.

This comment has been minimized.

@balloob

balloob Oct 2, 2018

Member
entry = entries_lookup.pop(entity.external_id, None)
if entry is None:
    # Remove obsolutee entity
    continue

# Existing entity - update details

This comment has been minimized.

@exxamalte

exxamalte Oct 2, 2018

Contributor

OK

def device_state_attributes(self):
"""Return the device state attributes."""
attributes = {}
if self.external_id:

This comment has been minimized.

@balloob

balloob Oct 2, 2018

Member

In case you feel like removing some repeating code:

for key, value in (
    (ATTR_EXTERNAL_ID, self.external_id),
    (ATTR_CATEGORY, self. _category),
):
    if value:
        attributes[key] = value

This comment has been minimized.

@exxamalte

exxamalte Oct 2, 2018

Contributor

Sounds good, will do.

exxamalte added some commits Sep 22, 2018

@exxamalte

This comment has been minimized.

Contributor

exxamalte commented Oct 13, 2018

Last update in this is to make it conform with PR #17339.

@MartinHjelmare

I think we can make the code more clear and efficient by taking more advantage of the uniqueness property of the external_id. We can use a set to store these representing existing entities. We can store the incoming feed entries as a dict with external_id as key. And we use our dispatch helper to signal the correct entities.

Then on update its very easy to match existing external_ids (entities) with incoming entries ids since sets support intersection and difference operations. We just create a temporary set from the incoming external_ids and compare this with existing external_ids.

Existing external_ids not in incoming external_ids should be removed (those entities should be signaled to be removed). Existing external_ids present in incoming external_ids should be updated (those entities should be signaled to be updated). Incoming external_ids not in existing external_ids should be used to create new entities.

This suggestion goes beyond what I've commented on below.

continue
# Existing entity - update details.
_LOGGER.debug("Existing entity found %s", entity)
feed_entries.remove(entry)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 13, 2018

Member

Remove this.

This comment has been minimized.

@exxamalte

exxamalte Oct 13, 2018

Contributor

Refactored to use set difference and intersection methods now.

feed_entries.remove(entry)
entity.schedule_update_ha_state(True)
# Return the remaining entries that new entities must be created for.
return feed_entries

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 13, 2018

Member

Return the remaining keys of entries_lookup instead. We only need to know what external_ids remains so we can pass these to the new entities.

This comment has been minimized.

@exxamalte

exxamalte Oct 13, 2018

Contributor

Refactored to use set difference and intersection methods now.

if status == geojson_client.UPDATE_OK:
_LOGGER.debug("Data retrieved %s", feed_entries)
# Keep a copy of all feed entries for future lookups by entities.
self._feed_entries = feed_entries.copy()

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 13, 2018

Member

We could make self._feed_entries a dict instead like entries_lookup and store that. We would benefit from this by not needing get_feed_entry method.

This comment has been minimized.

@exxamalte

exxamalte Oct 13, 2018

Contributor

OK. Are you suggesting to just expose feed_entries as a dict to the entities directly to be able to remove the get_feed_entry method?

class NswRuralFireServiceLocationEvent(GeoLocationEvent):
"""This represents an external event with GeoJSON data."""
def __init__(self, feed_manager, feed_entry):

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 13, 2018

Member

Pass in the external_id instead of the feed_entry and store the external_id as an instance attribute in init. We will update and get the feed entry after entity addition anyway.

This comment has been minimized.

@exxamalte

exxamalte Oct 13, 2018

Contributor

OK - only passing in external id now

def __init__(self, feed_manager, feed_entry):
"""Initialize entity with data from feed entry."""
self._feed_manager = feed_manager
self._update_from_feed(feed_entry)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 13, 2018

Member

Remove this. We will update after entity addition.

This comment has been minimized.

@exxamalte

exxamalte Oct 13, 2018

Contributor

OK

self._distance = feed_entry.distance_to_home
self._latitude = feed_entry.coordinates[0]
self._longitude = feed_entry.coordinates[1]
self.external_id = feed_entry.external_id

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 13, 2018

Member

We should never change the external_id for this entity.

This comment has been minimized.

@exxamalte

exxamalte Oct 13, 2018

Contributor

Yes - makes much more sense now after refactoring

self._add_entities(new_entities, True)
self._managed_entities.extend(new_entities)
def get_feed_entry(self, external_id):

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 13, 2018

Member

We can remove this.

This comment has been minimized.

@exxamalte

exxamalte Oct 13, 2018

Contributor

I'm not sure how to remove this. During an update, the entity still only has the external id to retrieved the full feed entry from the manager. Or, have I missed something?

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 13, 2018

Member

We could make self._feed_entries non private.

This comment has been minimized.

@exxamalte

exxamalte Oct 14, 2018

Contributor

Makes sense.

# Return the remaining entries that new entities must be created for.
return feed_entries
def _generate_new_entities(self, entries):

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 13, 2018

Member

Accept external_ids instead.

This comment has been minimized.

@exxamalte

exxamalte Oct 13, 2018

Contributor

OK

new_entities.append(new_entity)
# Add new entities to HA and keep track of them in this manager.
self._add_entities(new_entities, True)
self._managed_entities.extend(new_entities)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 13, 2018

Member

Instead of storing the entities we could store the external_ids in a set. We would then use our dispatch helper to signal the correct entity methods as needed. The signal id would be the external_id. The entities connect their update and remove methods to their corresponding signals when they have been added to home assistant.

This comment has been minimized.

@exxamalte

exxamalte Oct 13, 2018

Contributor

I added update and remove signals, and have hopefully used the dispatcher correctly.

exxamalte added some commits Oct 13, 2018

self._feed_entries = {entry.external_id: entry
for entry in feed_entries}
# For entity management the external ids from the feed are used.
feed_external_ids = {entry.external_id

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 13, 2018

Member

I think we can just do:

feed_external_ids = set(self._feed_entries)

This comment has been minimized.

@exxamalte

exxamalte Oct 14, 2018

Contributor

I didn't know about this trick.

async def async_added_to_hass(self):
"""Call when entity is added to hass."""
async_dispatcher_connect(
self.hass, SIGNAL_DELETE_ENTITY, self._delete_callback)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 13, 2018

Member

Make external_id part of the signal to avoid having to check the external_id in the callbacks. See eg:
https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/components/mysensors/device.py#L108.

This comment has been minimized.

@exxamalte

exxamalte Oct 14, 2018

Contributor

Makes sense, thanks for the hint.

self._add_entities(new_entities, True)
self._managed_entities.extend(new_entities)
def get_feed_entry(self, external_id):

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 13, 2018

Member

We could make self._feed_entries non private.

exxamalte added some commits Oct 14, 2018

@MartinHjelmare

Looks great!

@MartinHjelmare

This comment has been minimized.

Member

MartinHjelmare commented Oct 14, 2018

Can be merged when build passes.

@exxamalte

This comment has been minimized.

Contributor

exxamalte commented Oct 14, 2018

Thanks. Once this is through, I'll revisit the geo_json_events platform and refactor it in the same way. These can then be the blueprint for many more platforms.

@MartinHjelmare MartinHjelmare merged commit fccaf7f into home-assistant:dev Oct 14, 2018

5 checks passed

Hound No violations found. Woof!
WIP ready for review
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.02%) to 93.548%
Details

@wafflebot wafflebot bot removed the in progress label Oct 14, 2018

@balloob balloob referenced this pull request Oct 26, 2018

Merged

0.81 #17809

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