GeoJSON platform #16610

exxamalte · 2018-09-13T22:28:19Z

Description:

This is the first platform of the new geo location component. It fetches data from a GeoJSON feed and generates entities for each external event that is in the vicinity of the Home Assistant server. The radius around Home Assistant is configurable, and so is the scan interval.

I tried to keep this as simple as possible. Future enhancements may include the ability to access other properties of the feed or the ability to define additional filters.
More platforms for specific GeoJSON feeds are in the making, and so are similar platforms for GeoRSS feeds.

Related issue (if applicable): evolving #15953

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

Example entry for `configuration.yaml` (if applicable):

geo_location:
  - platform: geo_json_events
    url: http://www.rfs.nsw.gov.au/feeds/majorIncidents.json

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:

Documentation added/updated in home-assistant.github.io

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.

exxamalte · 2018-09-14T08:48:53Z

@MartinHjelmare: Here comes the first platform for the new geo location component. I decided to go with GeoJSON feeds first before looking at GeoRSS again, because GeoJSON is a bit simpler and does not depend on the feedreader component.

MartinHjelmare

I didn't look at the tests.

Please rename device to entity consistently cause that's what is referred to here.

MartinHjelmare · 2018-09-14T09:19:38Z

homeassistant/components/geo_location/geo_json_events.py

+from homeassistant.components.sensor.rest import RestData
+from homeassistant.const import CONF_RADIUS, CONF_URL, CONF_SCAN_INTERVAL, \
+    EVENT_HOMEASSISTANT_START, ATTR_ID, LENGTH_KILOMETERS, LENGTH_METERS
+from homeassistant.helpers.config_validation import PLATFORM_SCHEMA


Import this from the geo_location component.

MartinHjelmare · 2018-09-14T09:21:00Z

homeassistant/components/geo_location/geo_json_events.py

+    vol.Required(CONF_URL): cv.string,
+    vol.Optional(CONF_RADIUS, default=DEFAULT_RADIUS_IN_KM):
+        vol.Coerce(float),
+    vol.Optional(CONF_SCAN_INTERVAL, default=DEFAULT_SCAN_INTERVAL):


This is already present in the base platform config schema. Define the constant SCAN_INTERVAL to set default scan interval for this platform.

MartinHjelmare · 2018-09-14T09:22:14Z

homeassistant/components/geo_location/geo_json_events.py

+})
+
+
+def setup_platform(hass, config, add_entities, disc_info=None):


Please rename disc_info to discovery_info to keep variable names consistent all over home assistant.

MartinHjelmare · 2018-09-14T09:26:36Z

homeassistant/components/geo_location/geo_json_events.py

+
+import homeassistant.helpers.config_validation as cv
+from homeassistant.components.geo_location import GeoLocationEvent
+from homeassistant.components.sensor.rest import RestData


We shouldn't depend on another platform. Service I/O code should be hosted in a standalone library published on pypi.

OK, I understand the direction the project is taking here.
However, at this point there would really not be much more than what is currently implemented in the RestData class that would need to be outsourced.
And before I go down that path of an external library, could you please have a look at the GeoJsonDistanceHelper inside the platform. Would you expect that to be outsourced, too, or does this still count as data transformation and can stay inside the platform?

GeoJsonDistanceHelper certainly looks general enough to fit in a standalone library. I think that's a good idea.

Alright, let me take a look at moving the code.

In the meantime I moved all code that interacts with the external GeoJSON feed as well as the code that calculates distances into a separate library: https://pypi.org/project/geojson-client/

MartinHjelmare · 2018-09-14T09:29:54Z

homeassistant/components/geo_location/geo_json_events.py

+def setup_platform(hass, config, add_entities, disc_info=None):
+    """Set up the GeoJSON Events platform."""
+    url = config.get(CONF_URL)
+    scan_interval = config.get(CONF_SCAN_INTERVAL)


scan_interval = config.get(CONF_SCAN_INTERVAL, SCAN_INTERVAL)

MartinHjelmare · 2018-09-14T09:46:03Z

homeassistant/components/geo_location/geo_json_events.py

+                    # Update existing device's details with event data.
+                    latitude, longitude, _, name, category = \
+                        self._extract_data(feature)
+                    device.distance = entry[ATTR_DISTANCE]


We should invert the flow here so that the entity fetches the new state from a data holding instance in async_update. That's our standard. Use schedule_update_ha_state(True) on the entity.

OK. The manager class I implemented is currently fetching the data from the new library and can keep a copy of all entries. Each entity then fetches the original entry on async_update and updates its values.

MartinHjelmare · 2018-09-14T09:49:33Z

homeassistant/components/geo_location/geo_json_events.py

+        """Return the name of the entity."""
+        return self._name
+
+    @name.setter


Remove all the setters and invert the flow as commented above.

MartinHjelmare · 2018-09-14T09:52:59Z

homeassistant/components/geo_location/geo_json_events.py

+        return DEFAULT_UNIT_OF_MEASUREMENT
+
+    @property
+    def external_id(self):


This can be made a regular instance attribute.

MartinHjelmare · 2018-09-14T09:53:24Z

homeassistant/components/geo_location/geo_json_events.py

+        return self._external_id
+
+    @property
+    def category(self):


Same as above.

I removed the category altogether for this generic platform since there is no real standard defining categories in GeoJSON feeds.

MartinHjelmare · 2018-09-14T09:56:28Z

homeassistant/components/geo_location/geo_json_events.py

+    vol.Optional(CONF_SCAN_INTERVAL, default=DEFAULT_SCAN_INTERVAL):
+        cv.time_period,
+    vol.Optional(CONF_CATEGORIES, default=[]):
+        vol.All(cv.ensure_list, [cv.string]),


Please validate possible categories.

That won't be possible because the external GeoJSON feed can basically make up any category it wants.

Ok. How will the user know what to put here?

The user would need to look at what categories the provider of the GeoJSON feed has documented, or do a bit of reverse-engineering.

Is there an example of categories from some established feed?

Sure, for example this feed http://www.rfs.nsw.gov.au/feeds/majorIncidents.json uses the following categories:

Emergency Warning

Watch and Act

Advice

Not Applicable

Documented on the provider's overview page when you scroll down to the table "Alert Level".

I removed categories from this, and move this into a more specific platform, including validation.

exxamalte · 2018-09-14T17:18:13Z

@MartinHjelmare : Sorry, I just realised from some of your questions that it may make sense to briefly discuss the general direction for new platforms in the geo location component. Could you please have a look at the last comment in here: home-assistant/architecture#42

MartinHjelmare · 2018-09-14T17:33:05Z

Those are good questions. I don't have an opinion/answer at the moment.

exxamalte · 2018-09-18T00:19:51Z

@MartinHjelmare : As suggested I moved all code that accesses the external GeoJSON feed into a separate library. I refactored the code in this PR as requested and to integrate with the new library.
As suggested in the last comment in home-assistant/architecture#42, this PR may not go anywhere, but before I go ahead and integrate a first specific GeoJSON feed as a new platform, I was hoping you could take a quick look at this to confirm whether the refactoring go into the right direction? Thanks.

MartinHjelmare

Looks ok.

MartinHjelmare · 2018-09-18T02:50:17Z

homeassistant/components/geo_location/geo_json_events.py

+        """No polling needed for GeoJSON location events."""
+        return False
+
+    @asyncio.coroutine


Use Python 3.5 async syntax, ie async def.

MartinHjelmare · 2018-09-18T02:51:27Z

homeassistant/components/geo_location/geo_json_events.py

+
+def setup_platform(hass, config, add_entities, discovery_info=None):
+    """Set up the GeoJSON Events platform."""
+    url = config.get(CONF_URL)


Use dict[key] for required config keys.

…anups

…actored platform and tests to work with that new library

MartinHjelmare

Looks good!

MartinHjelmare · 2018-09-18T22:22:40Z

Maybe update the PR description about the category filter.

exxamalte · 2018-09-19T00:10:14Z

Updated the PR description. Doco will be added tonight.

homeassistant added integration: geo_location new-platform cla-signed labels Sep 13, 2018

ghost added the in progress label Sep 13, 2018

homeassistant added the cla-signed label Sep 13, 2018

MartinHjelmare requested changes Sep 14, 2018

View reviewed changes

homeassistant added the cla-signed label Sep 18, 2018

MartinHjelmare reviewed Sep 18, 2018

View reviewed changes

exxamalte added 6 commits September 18, 2018 21:24

initial version of geojson platform

5c2565f

unit tests for geo json platform added; smaller bugfixes and code cle…

e092b15

…anups

fixing pylint issues

e94bbdf

moved all code accessing the external feed into separate library; ref…

e30e77d

…actored platform and tests to work with that new library

fixing lint

16a438e

small refactorings

29d2ffa

exxamalte force-pushed the geo_location_geojson_platform branch from 6209ff4 to 29d2ffa Compare September 18, 2018 11:55

homeassistant added the cla-signed label Sep 18, 2018

MartinHjelmare approved these changes Sep 18, 2018

View reviewed changes

exxamalte mentioned this pull request Sep 19, 2018

GeoJSON platform home-assistant/home-assistant.io#6316

Merged

2 tasks

MartinHjelmare merged commit 18d37ff into home-assistant:dev Sep 21, 2018

ghost removed the in progress label Sep 21, 2018

balloob mentioned this pull request Sep 28, 2018

0.79.0 #16940

Merged

home-assistant locked and limited conversation to collaborators Feb 5, 2019

		})


		def setup_platform(hass, config, add_entities, disc_info=None):

GeoJSON platform #16610

GeoJSON platform #16610

Conversation

exxamalte commented Sep 13, 2018 • edited Loading

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

exxamalte commented Sep 14, 2018

MartinHjelmare left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartinHjelmare Sep 14, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

exxamalte commented Sep 14, 2018

MartinHjelmare commented Sep 14, 2018

exxamalte commented Sep 18, 2018

MartinHjelmare left a 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

MartinHjelmare commented Sep 18, 2018

exxamalte commented Sep 19, 2018

exxamalte commented Sep 13, 2018 •

edited

Loading

Example entry for `configuration.yaml` (if applicable):

MartinHjelmare Sep 14, 2018 •

edited

Loading