Add support for Zillow Zestimate sensor #12597

jcconnell · 2018-02-22T03:41:15Z

Description:

Adds support for the Zillow Zestimate as a sensor.

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

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

sensor:
  - platform: zestimate
    name: Zillow Zestimate
    api_key: <zillow API key>
    zpid:
      - <zpid 1>
      - <zpid 2>

Checklist:

The code change is tested and works locally.

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][ex-requir]).
New dependencies are only imported inside functions that use them ([example][ex-import]).
New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
New files were added to .coveragerc.

houndci-bot · 2018-02-22T03:41:23Z

homeassistant/components/sensor/zestimate.py

+            details[ATTR_CURRENCY] = data['amount']['@currency']
+            details[ATTR_LAST_UPDATED] = data['last-updated']
+            details[ATTR_CHANGE] = int(data['valueChange']['#text'])
+            details[ATTR_VAL_HIGH] = int(data['valuationRange']['high']['#text'])


line too long (81 > 79 characters)

tinloaf

Some comments. On a larger scale, I'm not sure why you split the whole thing into two classes. I think the complexity of the code is not that high, and splitting it over two classes makes things rather more complicated than easier - but that's just my taste I guess. :)

tinloaf · 2018-02-22T09:53:03Z

homeassistant/components/sensor/zestimate.py

+            self.address = response['address']['street']
+            self.measurings = details
+        except AttributeError:
+            self.measurings = None


You'd probably want to log an error (or at least a warning) here?

tinloaf · 2018-02-22T09:55:05Z

homeassistant/components/sensor/zestimate.py

+
+ICON = 'mdi:home-variant'
+
+ATTR_LOCATION = 'location'


ATTR_LOCATION is already defined in const.py.

tinloaf · 2018-02-22T10:01:27Z

homeassistant/components/sensor/zestimate.py

+    for zpid in properties:
+        params['zpid'] = zpid
+        try:
+            response = requests.get(_RESOURCE, params=params, timeout=5)


Why do you do this? This looks like you do the first update before you create the ZestimateData object. But you don't use any of the results to construct the entity?

Please don't do this. Setting up the platform should be fast, i.e., should do no I/O if not absolutely necessary. The first update should happen only when your update method is called for the first time.

tinloaf · 2018-02-22T10:02:26Z

homeassistant/components/sensor/zestimate.py

+        """Initialize the sensor."""
+        self.data = data
+        self._name = name
+        self.update()


Please don't call update here. Since you called add_devices with True as second parameter above, your update method will be called right after adding the entity anyways.

tinloaf · 2018-02-22T10:04:48Z

homeassistant/components/sensor/zestimate.py

+
+            attributes[ATTR_LOCATION] = self.data.address
+            attributes[ATTR_ATTRIBUTION] = CONF_ATTRIBUTION
+            return attributes


Event though None is the default return value if your method doesn't return anything, I think it's nicer to still explicitly return None if there are no measurings. It's easier to read that way.

tinloaf · 2018-02-22T10:05:32Z

homeassistant/components/sensor/zestimate.py

+    def update(self):
+        """Get the latest data and update the states."""
+        self.data.update()
+        if self.data.measurings is not None:


Shouldn't the state also be set to unknown if there are no measurings?

jcconnell · 2018-02-22T17:45:09Z

@tinloaf, thanks for the comments. I'm still learning and they are sincerely appreciated. I built it this way because the Swiss Hydrological Data sensor seemed like a component that was already solving a similar problem. I'll work to incorporate your changes and then push once complete.

jcconnell · 2018-02-26T21:30:55Z

@tinloaf Any comment on the changes I made?

MartinHjelmare · 2018-03-03T12:32:08Z

homeassistant/components/sensor/zestimate.py

+from datetime import timedelta
+
+import voluptuous as vol
+import requests


Please sort 🔤.

MartinHjelmare · 2018-03-03T12:34:07Z

homeassistant/components/sensor/zestimate.py

+        try:
+            return round(float(self._state), 1)
+        except ValueError:
+            return STATE_UNKNOWN


Return None . The base entity class will handle unknown state.

MartinHjelmare · 2018-03-03T12:34:36Z

homeassistant/components/sensor/zestimate.py

+        if self.data is not None:
+            self._state = self.data[ATTR_AMOUNT]
+        else:
+            self._state = STATE_UNKNOWN


Return None.

MartinHjelmare · 2018-03-03T12:35:27Z

homeassistant/components/sensor/zestimate.py

+        """Return the state attributes."""
+        attributes = {}
+        if self.data is not None:
+            attributes['Amount'] = self.data[ATTR_AMOUNT]


Attributes should be lowercase snakecase.

It seems self.data is a subset of attributes, so just update attributes with self.data and then set address and attribution.

MartinHjelmare · 2018-03-03T12:36:08Z

homeassistant/components/sensor/zestimate.py

+            attributes['Valuation Range Low'] = self.data[ATTR_VAL_LOW]
+        attributes['Address'] = self.address
+        attributes[ATTR_ATTRIBUTION] = CONF_ATTRIBUTION
+        if attributes is not None:


The other case will never happen.

MartinHjelmare · 2018-03-03T12:38:01Z

homeassistant/components/sensor/zestimate.py

+            if error_code != 0:
+                _LOGGER.error('The API returned: %s',
+                              data_dict['message']['text'])
+                return False


Nothing is checking this return value. Just return.

MartinHjelmare · 2018-03-03T12:38:09Z

homeassistant/components/sensor/zestimate.py

+                return False
+        except requests.exceptions.ConnectionError:
+            _LOGGER.error('Unable to retrieve data from %s', _RESOURCE)
+            return False


Same as above.

MartinHjelmare · 2018-03-03T12:42:45Z

homeassistant/components/sensor/zestimate.py

+
+
+def setup_platform(hass, config, add_devices, discovery_info=None):
+    """Setup the Zestimate sensor."""


"Set up the..."

jcconnell · 2018-03-03T15:35:11Z

Thank you @MartinHjelmare! I think I've addressed everything you requested.

MartinHjelmare

One fix left to do, otherwise looks good!

MartinHjelmare · 2018-03-03T15:37:19Z

homeassistant/components/sensor/zestimate.py

+"""
+from datetime import timedelta
+import logging
+import requests


Add a blank line between standard library and 3rd party imports.

MartinHjelmare

Looks great!

jcconnell · 2018-03-03T16:41:55Z

Thank you!

jcconnell added 2 commits February 22, 2018 03:26

Added Zillow Zestimate sensor.

89eb0e1

Add zestimate.py to .coveragerc

9a07396

jcconnell requested a review from andrey-git as a code owner February 22, 2018 03:41

homeassistant added cla-signed new-platform platform: sensor.zestimate labels Feb 22, 2018

houndci-bot reviewed Feb 22, 2018

View reviewed changes

Fix line 167 81>80

14b1006

homeassistant added the cla-signed label Feb 22, 2018

tinloaf suggested changes Feb 22, 2018

View reviewed changes

Incorporate tinloaf changes.

28e2567

homeassistant added the cla-signed label Feb 23, 2018

Saving work

f1d8b5b

MartinHjelmare requested changes Mar 3, 2018

View reviewed changes

MartinHjelmare changed the title ~~Adds support for Zillow Zestimate sensor~~ Add support for Zillow Zestimate sensor Mar 3, 2018

Incorporate changes requested by MartinHjelmare

ed7d885

homeassistant added the cla-signed label Mar 3, 2018

Remove unnecessary import

3efe493

MartinHjelmare requested changes Mar 3, 2018

View reviewed changes

Add a blank line between standard library and 3rd party imports

aa05824

homeassistant added the cla-signed label Mar 3, 2018

home-assistant deleted a comment from houndci-bot Mar 3, 2018

MartinHjelmare approved these changes Mar 3, 2018

View reviewed changes

MartinHjelmare merged commit a9d242a into home-assistant:dev Mar 3, 2018

balloob mentioned this pull request Mar 9, 2018

0.65 #12995

Merged

home-assistant locked and limited conversation to collaborators Jul 26, 2018

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Zillow Zestimate sensor #12597

Add support for Zillow Zestimate sensor #12597

jcconnell commented Feb 22, 2018 •

edited

Loading

houndci-bot Feb 22, 2018

tinloaf left a comment

tinloaf Feb 22, 2018

tinloaf Feb 22, 2018

tinloaf Feb 22, 2018

tinloaf Feb 22, 2018

tinloaf Feb 22, 2018

tinloaf Feb 22, 2018

jcconnell commented Feb 22, 2018

jcconnell commented Feb 26, 2018

MartinHjelmare Mar 3, 2018

MartinHjelmare Mar 3, 2018

MartinHjelmare Mar 3, 2018

MartinHjelmare Mar 3, 2018

MartinHjelmare Mar 3, 2018

MartinHjelmare Mar 3, 2018

MartinHjelmare Mar 3, 2018

MartinHjelmare Mar 3, 2018

MartinHjelmare Mar 3, 2018

jcconnell commented Mar 3, 2018

MartinHjelmare left a comment

MartinHjelmare Mar 3, 2018

MartinHjelmare left a comment

jcconnell commented Mar 3, 2018



		def setup_platform(hass, config, add_devices, discovery_info=None):
		"""Setup the Zestimate sensor."""


		ICON = 'mdi:home-variant'

		ATTR_LOCATION = 'location'

Add support for Zillow Zestimate sensor #12597

Add support for Zillow Zestimate sensor #12597

Conversation

jcconnell commented Feb 22, 2018 • edited Loading

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

Choose a reason for hiding this comment

tinloaf 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

jcconnell commented Feb 22, 2018

jcconnell commented Feb 26, 2018

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

jcconnell commented Mar 3, 2018

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

jcconnell commented Mar 3, 2018

jcconnell commented Feb 22, 2018 •

edited

Loading

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