Netatmo public by colinfrei · Pull Request #15684 · home-assistant/core

colinfrei · 2018-07-25T20:53:02Z

Description:

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

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

sensor:
  - platform: netatmo_public
    areas:
      - lat_ne: 40.719
        lon_ne: -73.735
        lat_sw: 40.552
        lon_sw: -74.105

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.
New files were added to .coveragerc.

If the code does not interact with devices:

Tests have been added to verify that the new code works.

colinfrei · 2018-07-25T21:04:16Z

I linked my git@ email address to github as well, hope the cla-bot rechecks that automatically :)

homeassistant · 2018-07-26T02:02:12Z

Hi @colinfrei,

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!

Danielhiversen · 2018-07-27T07:12:35Z

Are all the public sensors for rain? Never temperature?

colinfrei · 2018-07-27T07:15:29Z

I use the rain sensors, which is why I added those, temperature and some others would be available as well.

Danielhiversen · 2018-07-28T06:20:00Z

Ok, so where are you specifying that you get rain data or is that default?

Danielhiversen

Thanks for your contribution. This will be a useful component.

I have added some comments.
You also got some comments on your documentation: home-assistant/home-assistant.io#5893 (comment)

Danielhiversen · 2018-07-31T15:48:42Z

homeassistant/components/sensor/netatmo_public.py

+
+
+def calculate_average(raindata):
+    """Get the average value in the area."""


Why not use sum(raindata.values()) / len(raindata) ?

Danielhiversen · 2018-07-31T16:01:33Z

homeassistant/components/sensor/netatmo_public.py

+                                     LAT_NE=self.lat_ne,
+                                     LON_NE=self.lon_ne,
+                                     LAT_SW=self.lat_sw,
+                                     LON_SW=self.lon_sw)


Add required_data_type = "rain" to make it obvious that you ask for rain data

Danielhiversen · 2018-07-31T16:02:01Z

homeassistant/components/sensor/netatmo_public.py

+        self._state = self.netatmo_data.data
+
+
+class NetatmoPublicData(object):


This NetatmoPublicData class is not needed when we only support rain data

Would it be ok to leave it since I'd expect that to expand to additional metrics soon?

I think so. Would be nice with other sensors too.

Danielhiversen · 2018-07-31T16:09:16Z

homeassistant/components/sensor/netatmo_public.py

+
+        if raindata.CountStationInArea() == 0:
+            _LOGGER.warning('No Rain Station available in this area.')
+        else:


Use return here, and skip the else:

Danielhiversen · 2018-07-31T16:11:12Z

homeassistant/components/sensor/netatmo_public.py

+
+def calculate_max(raindata):
+    """Get the highest value in the area."""
+    key_max = max(raindata.keys(), key=(lambda k: raindata[k]))


Why not max(raindata.values()) ?

Danielhiversen · 2018-07-31T16:12:15Z

homeassistant/components/sensor/netatmo_public.py

+            raindata_live = raindata.getLive()
+
+            if self.calculation == 'avg':
+                self.data = calculate_average(raindata_live)


Not necessary to calculate_average as a separate function.

Danielhiversen · 2018-07-31T16:12:25Z

homeassistant/components/sensor/netatmo_public.py

+            if self.calculation == 'avg':
+                self.data = calculate_average(raindata_live)
+            else:
+                self.data = calculate_max(raindata_live)


Not necessary to calculate_max as a separate function.

houndci-bot · 2018-08-05T12:42:51Z

homeassistant/components/sensor/netatmo_public.py

+                                     LON_NE=self.lon_ne,
+                                     LAT_SW=self.lat_sw,
+                                     LON_SW=self.lon_sw,
+                                     required_data_type = "rain")


unexpected spaces around keyword / parameter equals

christ0ph · 2018-08-06T19:53:38Z

Ah, this one would be so usefull!

Danielhiversen

LGTM. 🎉
Just remove the error logging line
And fix the style error: https://travis-ci.org/home-assistant/home-assistant/jobs/412315360#L565

Danielhiversen · 2018-08-06T19:58:56Z

homeassistant/components/sensor/netatmo_public.py

+        self._state = self.netatmo_data.data
+
+
+class NetatmoPublicData(object):


I think so. Would be nice with other sensors too.

Danielhiversen · 2018-08-06T19:59:24Z

homeassistant/components/sensor/netatmo_public.py

+
+        raindata_live = raindata.getLive()
+
+        _LOGGER.error(raindata_live.values())


Should be removed?

…instructions

colinfrei · 2018-08-07T08:36:47Z

Fixed :)

MartinHjelmare · 2018-08-14T07:28:51Z

homeassistant/components/sensor/netatmo_public.py

+    def update(self):
+        """Request an update from the Netatmo API."""
+        import pyatmo
+        _LOGGER.error('Updating Netatmo data.')


Wrong log level. We can remove this all together.

definitely - not sure how that sneaked in.
Fixed here: #15966

* Add a sensor for netatmo public data * A bit of cleanup before submitting pull request * Add netatmo_public file to .coveragerc, as per pull request template instructions * Fixes for tox complaining * make calculations simpler, based on review feedback * explicitly pass required_data parameter to netatmo API * remove unnecessary spaces * remove debug code * code style fix

homeassistant added cla-error new-platform platform: sensor.netatmo_public labels Jul 25, 2018

ghost added the in progress label Jul 25, 2018

colinfrei mentioned this pull request Jul 25, 2018

Add documentation for netatmo public component home-assistant/home-assistant.io#5893

Merged

2 tasks

awarecan added cla-recheck and removed cla-error labels Jul 26, 2018

homeassistant added cla-needed cla-signed and removed cla-recheck cla-needed labels Jul 26, 2018

home-assistant deleted a comment from homeassistant Jul 26, 2018

Danielhiversen requested changes Jul 31, 2018

View reviewed changes

colinfrei force-pushed the netatmo-public branch from d8b6909 to a4f4e52 Compare August 5, 2018 10:05

homeassistant added the cla-signed label Aug 5, 2018

colinfrei force-pushed the netatmo-public branch from a4f4e52 to 391e8e9 Compare August 5, 2018 12:42

homeassistant added the cla-signed label Aug 5, 2018

houndci-bot reviewed Aug 5, 2018

View reviewed changes

Danielhiversen approved these changes Aug 6, 2018

View reviewed changes

colinfrei added 5 commits August 7, 2018 07:10

Add a sensor for netatmo public data

261cb70

A bit of cleanup before submitting pull request

fc789d2

Add netatmo_public file to .coveragerc, as per pull request template …

a7bffa0

…instructions

Fixes for tox complaining

78a8d63

make calculations simpler, based on review feedback

4f5a8e2

colinfrei added 4 commits August 7, 2018 07:10

explicitly pass required_data parameter to netatmo API

9c74c9b

remove unnecessary spaces

49d5aac

remove debug code

fd90ce8

code style fix

8afb891

colinfrei force-pushed the netatmo-public branch from 0ac6ee2 to 8afb891 Compare August 7, 2018 07:22

homeassistant added the cla-signed label Aug 7, 2018

fabaff merged commit f411fb8 into home-assistant:dev Aug 13, 2018

ghost removed the in progress label Aug 13, 2018

MartinHjelmare reviewed Aug 14, 2018

View reviewed changes

colinfrei mentioned this pull request Aug 14, 2018

Remove unnecessary log #15966

Merged

8 tasks

balloob mentioned this pull request Aug 29, 2018

0.77 #16256

Merged

home-assistant locked and limited conversation to collaborators Dec 10, 2018

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



		def calculate_average(raindata):
		"""Get the average value in the area."""

		self._state = self.netatmo_data.data


		class NetatmoPublicData(object):


		raindata_live = raindata.getLive()

		_LOGGER.error(raindata_live.values())

Uh oh!

Conversation

colinfrei commented Jul 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

Uh oh!

colinfrei commented Jul 25, 2018

Uh oh!

homeassistant commented Jul 26, 2018

Uh oh!

Danielhiversen commented Jul 27, 2018

Uh oh!

colinfrei commented Jul 27, 2018

Uh oh!

Danielhiversen commented Jul 28, 2018

Uh oh!

Danielhiversen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christ0ph commented Aug 6, 2018

Uh oh!

Danielhiversen left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

colinfrei commented Aug 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

colinfrei commented Jul 25, 2018 •

edited

Loading

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

Danielhiversen left a comment •

edited

Loading