Added Met Office weather and sensor components #6742

jacobtomlinson · 2017-03-22T20:45:17Z

Description:
Get weather data from the Met Office's DataPoint API.

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

Example entry for configuration.yaml (if applicable):

sensor:
  - platform: metoffice
    api_key: "myapikey"
    monitored_conditions:
      - temperature
      - weather

weather:
  - platform: metoffice
    api_key: "myapikey"

Checklist:

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:

Local tests with tox run successfully. Your PR cannot be merged unless tests pass
New dependencies have been added to the REQUIREMENTS variable (example).
New dependencies are only imported inside functions that use them (example).
New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
New files were added to .coveragerc.

homeassistant · 2017-03-22T20:45:19Z

Hi @jacobtomlinson,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

houndci-bot · 2017-03-22T20:45:27Z

homeassistant/components/sensor/metoffice.py

+"""
+
+import logging
+import datetime


'datetime' imported but unused

jacobtomlinson · 2017-03-22T22:07:39Z

This has been raised to replace #5463. However it's not 100% finished and is still undergoing some testing.

jacobtomlinson · 2017-03-23T21:01:13Z

Good to go at my end

fabaff · 2017-03-23T21:39:34Z

homeassistant/components/sensor/metoffice.py

+REQUIREMENTS = ['datapoint==0.4.3']
+
+CONF_ATTRIBUTION = "Data provided by the Met Office"
+CONF_MO_API_KEY = 'api_key'


We have CONF_API_KEY in const.py. i don't see a good reason to introduce CONF_MO_API_KEY.

fabaff · 2017-03-23T21:44:41Z

homeassistant/components/sensor/metoffice.py

+    'weather': ['Weather', None],
+    'temperature': ['Temperature', TEMP_CELSIUS],
+    'feels_like_temperature': ['Feels Like Temperature', TEMP_CELSIUS],
+    'wind_speed': ['Wind Speed', 'mps'],


Other platforms are using m/s.

fabaff · 2017-03-23T21:46:57Z

homeassistant/components/sensor/metoffice.py

+    @property
+    def name(self):
+        """Return the name of the sensor."""
+        return 'Met Office {}'.format(SENSOR_TYPES[self._condition][0])


Often DEFAULT_NAME is used if the user is not giving a name.

This is in keeping with the bom component. Also your comment here is at odds with your comment on the weather component about using CONF_NAME.

Do you suggest using CONF_NAME if it has been set and DEFAULT_NAME otherwise?

This is useful for users who are enabling multiple platform.

Ok I'll get that done.

fabaff · 2017-03-24T08:02:37Z

homeassistant/components/weather/metoffice.py

+Support for UK Met Office weather service.
+
+For more details about this platform, please refer to the documentation at
+https://home-assistant.io/components/sensor.metoffice/


Link needs to match the component otherwise the error message of the configuration check points to the wrong location. sensor.* -> weather.*

fabaff · 2017-03-24T08:05:13Z

homeassistant/components/weather/metoffice.py

+    datapoint = dp.connection(api_key=config.get(CONF_MO_API_KEY))
+
+    if None in (hass.config.latitude, hass.config.longitude):
+        _LOGGER.error("Latitude or longitude not set in Home Assistant config")


To make this platform more generic, I would like to suggest that the user can specify the location in the configuration and use the data from a different location.

Agreed. Will add this.

Done. User's can specify latitude and longitude in the component configs, and if they do not it will default to their home latitude and longitude.

Documentation also updated in home-assistant/home-assistant.io#2318.

fabaff · 2017-03-24T08:09:47Z

homeassistant/components/weather/metoffice.py

+    @property
+    def condition(self):
+        """Return the current condition."""
+        return self.data.data.weather.text


The condition should be mapped to an entry from CONDITION_CLASSES. This will make it possible in the future to show a visual representation of the condition in the frontend.

Agreed, will add this.

fabaff · 2017-03-24T08:10:39Z

homeassistant/components/weather/metoffice.py

+    @property
+    def name(self):
+        """Return the name of the sensor."""
+        return 'Met Office ({})'.format(self.site.name)


Using CONF_NAME would give the user more flexibility.

See comment on other name property review.

fabaff · 2017-03-24T08:11:40Z

homeassistant/components/sensor/metoffice.py

+        self._datapoint = datapoint
+        self._site = site
+        self.data = None
+        self.lastupdate = LAST_UPDATE


Is this used?

no, removed

fabaff · 2017-03-24T08:13:49Z

homeassistant/components/sensor/metoffice.py

+    if not site:
+        _LOGGER.error("Unable to get nearest Met Office forecast site")
+        return False
+    else:


We can drop the else and adjust the indent since the if statement returns False.

fabaff · 2017-03-24T08:15:51Z

homeassistant/components/weather/metoffice.py

+from homeassistant.components.weather import WeatherEntity, PLATFORM_SCHEMA
+from homeassistant.const import CONF_NAME, TEMP_CELSIUS
+from homeassistant.helpers import config_validation as cv
+# Reuse data and API logic from the sensor implementation


From my point of view it should be the other way around. The weather platform should act as the primary implementation and the sensor platform as a dependent.

I'm not sure I agree. I admit the reason I did it this way around is because it's the way the bom component did it. However it feels like the weather component is a collection of sensors, therefore it makes sense for the weather component to import the sensors.

We created the weather component to replace the sensor platforms for meteorological data.

So, I tend to say that the weather component should be the main implementation because there will be a frontend element some day and the sensor platforms will be obsolete. But as we are moving slowing in this regard I will leave it up to you to decide what platform should be the main implementation.

Thanks for explaining, makes a lot of sense. I'll update it so the weather component is the primary.

houndci-bot · 2017-03-24T10:12:03Z

homeassistant/components/weather/metoffice.py

+
+from homeassistant.components.weather import WeatherEntity, PLATFORM_SCHEMA
+from homeassistant.const import CONF_NAME, TEMP_CELSIUS, CONF_API_KEY,
+    CONF_LATITUDE, CONF_LONGITUDE


IndentationError: unexpected indent
unexpected indentation

houndci-bot · 2017-03-24T10:12:03Z

homeassistant/components/sensor/metoffice.py

+    datapoint = dp.connection(api_key=config.get(CONF_API_KEY))
+
+    latitude = config.get(CONF_LATITUDE, hass.config.latitude)
+    longitude = config.get(CONF_LONGITUDE, hass.config.longitude)


undefined name 'CONF_LONGITUDE'

houndci-bot · 2017-03-24T10:12:03Z

homeassistant/components/sensor/metoffice.py

+    import datapoint as dp
+    datapoint = dp.connection(api_key=config.get(CONF_API_KEY))
+
+    latitude = config.get(CONF_LATITUDE, hass.config.latitude)


undefined name 'CONF_LATITUDE'

jacobtomlinson · 2017-03-24T10:35:20Z

@fabaff I've finished working through your review. There were three items I disagreed with or had questions about. Would you mind updating these?

cannfoddr · 2017-05-03T17:58:19Z

This is not working for me and a few others over on the forums. Where can I log an Issue with the developer?

jacobtomlinson · 2017-05-03T18:39:07Z

That would be me! Send me a link to the thread and I'll take a look.

…

On Wed, 3 May 2017 at 18:58, cannfoddr ***@***.***> wrote: This is not working for me and a few others over on the forums. Where can I log an Issue with the developer? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6742 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABiUYvXlc8VKq0n3QEkQfy56gY37kiKeks5r2MBHgaJpZM4Mly_C> .

cannfoddr · 2017-05-03T20:12:19Z

Take a look here: https://community.home-assistant.io/t/metoffice-integration/15537

…

Sent from my iPhone

On 3 May 2017, at 19:39, Jacob Tomlinson ***@***.***> wrote: That would be me! Send me a link to the thread and I'll take a look. On Wed, 3 May 2017 at 18:58, cannfoddr ***@***.***> wrote: > This is not working for me and a few others over on the forums. Where can > I log an Issue with the developer? > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#6742 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/ABiUYvXlc8VKq0n3QEkQfy56gY37kiKeks5r2MBHgaJpZM4Mly_C> > . > — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

cannfoddr · 2017-05-05T09:43:58Z

Take a look here: https://community.home-assistant.io/t/metoffice-integration/15537 <https://community.home-assistant.io/t/metoffice-integration/15537>

…

On 3 May 2017, at 19:39, Jacob Tomlinson ***@***.***> wrote: That would be me! Send me a link to the thread and I'll take a look. On Wed, 3 May 2017 at 18:58, cannfoddr ***@***.***> wrote: > This is not working for me and a few others over on the forums. Where can > I log an Issue with the developer? > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#6742 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/ABiUYvXlc8VKq0n3QEkQfy56gY37kiKeks5r2MBHgaJpZM4Mly_C> > . > — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#6742 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA_LOh1OKOUbCdabrCNxMbrF7beDrgfLks5r2MnTgaJpZM4Mly_C>.

homeassistant added new-platform platform: sensor.metoffice labels Mar 22, 2017

homeassistant added the cla-needed label Mar 22, 2017

houndci-bot reviewed Mar 22, 2017

View reviewed changes

homeassistant added cla-signed and removed cla-needed labels Mar 22, 2017

balloobbot added the platform label Mar 22, 2017

homeassistant added the cla-signed label Mar 22, 2017

balloobbot added the requirement label Mar 23, 2017

jacobtomlinson force-pushed the metoffice branch from e9275f8 to ae6996f Compare March 23, 2017 20:32

homeassistant added the cla-signed label Mar 23, 2017

jacobtomlinson added 5 commits March 23, 2017 20:33

Added Met Office weather and sensor components

966c760

Removed unnecessary dependancy

32c1588

Generated requirements

3675888

Fix time interval

160ef4f

Updated coverage

940f57d

jacobtomlinson force-pushed the metoffice branch from ae6996f to 940f57d Compare March 23, 2017 20:34

homeassistant added the cla-signed label Mar 23, 2017

jacobtomlinson mentioned this pull request Mar 23, 2017

Add metoffice home-assistant/home-assistant.io#2318

Closed

fabaff requested changes Mar 24, 2017

View reviewed changes

jacobtomlinson added 2 commits March 24, 2017 08:45

Some review changes

8e45832

Allow user to specify lat and lon in component

9bae059

houndci-bot reviewed Mar 24, 2017

View reviewed changes

jacobtomlinson added 2 commits March 24, 2017 10:18

Added missing import

82efcfd

Fixed unit

4978e50

jacobtomlinson added 2 commits March 24, 2017 10:20

Fixed import indent

74faf22

Updated condition to use CONDITION_CLASSES

c82f42c

fabaff approved these changes Mar 31, 2017

View reviewed changes

fabaff merged commit 2d6b095 into home-assistant:dev Mar 31, 2017

fabaff mentioned this pull request Apr 6, 2017

0.42 #6956

Merged

home-assistant locked and limited conversation to collaborators Aug 12, 2017

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

Added Met Office weather and sensor components #6742

Added Met Office weather and sensor components #6742

Conversation

jacobtomlinson commented Mar 22, 2017 • edited

homeassistant commented Mar 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobtomlinson commented Mar 22, 2017

jacobtomlinson commented Mar 23, 2017

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

jacobtomlinson Mar 24, 2017 • edited

Choose a reason for hiding this comment

fabaff Mar 26, 2017 • edited

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

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

jacobtomlinson commented Mar 24, 2017

cannfoddr commented May 3, 2017

jacobtomlinson commented May 3, 2017 via email

cannfoddr commented May 3, 2017 via email

cannfoddr commented May 5, 2017 via email

jacobtomlinson commented Mar 22, 2017 •

edited

jacobtomlinson Mar 24, 2017 •

edited

fabaff Mar 26, 2017 •

edited