Refactor Forecast.io #2217

oudeismetis · 2016-06-02T23:50:25Z

Description: Connect to #2195 - The refactors the component to improve readability and maintainability. #2227 is being worked in tandem to test this component. The ultimate goal is to ensure there are no accidental future code changes that result in users getting high rate charges ($$$) from forecast.io.

Related issue (if applicable): fixes #2190
Connected to #2195 and #2227

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

Example entry for configuration.yaml (if applicable):

Checklist:

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

Documentation added/updated in home-assistant.io

If code communicates with devices:

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.

If the code does not interact with devices:

Local tests with tox run successfully. Your PR cannot be merged unless tests pass
Tests have been added to verify that the new code works.

rmkraus · 2016-06-03T00:03:56Z

homeassistant/components/sensor/forecast.py

-            "Connection error "
-            "Please check your settings for Forecast.io.")
+    api_key = config.get(CONF_API_KEY, None)
+    if None in (hass.config.latitude, hass.config.longitude, api_key):


Check out the validate_config function for this.

I was able to use validate_config for the API key, but couldn't figure out how to use it for a HA level config. All the example I found were just checking manually like above

rmkraus · 2016-06-03T00:12:05Z

Other than that one comment, it looks good to me.

oudeismetis · 2016-06-03T02:12:01Z

Sounds Good. I also want to double check that it's still not calling the API too many times. While I'm at it, I'll add a few code comments.

fabaff · 2016-06-03T12:17:34Z

homeassistant/components/sensor/forecast.py

-            "Please check your settings for Forecast.io.")
+    api_key = config.get(CONF_API_KEY, None)
+    if None in (hass.config.latitude, hass.config.longitude, api_key):
+        _LOGGER.error("Latitude, longitude, or API key missing from config")


I still think that the latitude/longitude error should be separate from the API key one. The settings for the latitude and longitude are configuration entries for HA itself while the API key is related to the configuration of the sensor thus they have different positions in the configuration.yaml file. This could mislead users.

Makes perfect sense. I'll change that.

oudeismetis · 2016-06-07T14:59:50Z

Still working on this.
In the process of attempting to convert the massive else if block to a psudo-switch, I discovered a data type that forecast.io has listed in their API doc, but which they don't return a value for. This causes everything to break for the basic dictionary switch, but only 1 thing to break otherwise.

Currently working on making this more resilient to API changes like that.

kellerza · 2016-06-11T17:26:49Z

Does #2195 not fix the problem in the description?

oudeismetis · 2016-06-11T17:37:02Z

@kellerza it does, but there was discussion about how it was too easy for a small change to result in a huge increase in API calls.

2 solutions were discussed: a test to catch that, and a refactor to make it more obvious.

Ended up doing both. #2227 does the testing piece.

oudeismetis · 2016-06-11T17:40:57Z

homeassistant/components/sensor/forecast.py

+            'pressure': round(getattr(data, 'pressure', 0), 1),
+            'visibility': getattr(data, 'visibility', 0),
+            'ozone': round(getattr(data, 'ozone', 0), 1)
+        }.get(self.type, self._state)


This now works. catching PropertyUnavailable no longer relevant.

Two things to discuss:

I thought about converting snake case to camel case to further simplify the lookup. Problem is that half of the responses are treated like integers and need different operations done on them.

defaults. For the numbers, defaulting to zero works fine, but I imagine there could be an edge case where an api error results in a return that there is "zero percent chance of rain". Suddenly someone home automation setup kicks off the sprinklers during a downpour. Not sure if there are any thoughts on that.

I'm not the python authority, but it seems like you'll update all integers with this call and then only use the correct, or not?

The if/then will only calculate if that type is used

I think you can break out the dict from the method by mapping the type string to a tuple holding the data string and a lambda function. The lambda should process the data attribute correctly. So when the method to get current state is called, it first does the lookup, then gets the attribute, then processes the attribute.

Regarding sane defaults, it's probably hard to catch all side effects, when the weather changes.

If you also do the camelcase conversion in the method you can skip the tuple and just map the type string to the lambda function.

yea, I'm not a huge fan of lambda's when you have to put a bunch of them together like this.
I'm looking right now to see if there is anything else that can be done to tighten this up.
I do agree with @kellerza concern that each integer will get calculated every time. Good catch.

Seeing what I can do.

kellerza · 2016-06-11T18:05:31Z

@oudeismetis Makes more sense now, would be good to add that to the description :-)

kellerza · 2016-06-11T18:16:04Z

homeassistant/components/sensor/forecast.py

@@ -124,80 +116,66 @@ def state(self):
    @property
    def unit_of_measurement(self):
        """Return the unit of measurement of this entity, if any."""
-        return self._unit_of_measurement
+        self.forecast_data.update_unit_of_measurement(self.type)


Will the unit of measurement ever change during the lifetime of this object?
You also probably need to lock this in some way if you do it this way, since another property updating in another thread can also call this and the forecasr_data.unit of measurement will be changed to that type

As I understand it, no. Unit of measure is not likely to ever change for the running system.

best to make this return the unit of measurement and save it in the sensor's __init__

I think you're right

So once I looked at this, I realized that unit of measurement should NOT be on the data object as it is sensor type specific. So I moved it back up to the sensor level, but still implemented your recommendation and cleaned up some other things.

I think it makes more sense now. Let me know what you think @kellerza when I push in a minute.

balloob · 2016-06-13T00:23:27Z

I just merged the tests. Can you rebase on dev to make sure everything still works?

balloob · 2016-06-13T00:27:25Z

homeassistant/components/sensor/forecast.py

+        # same exact call, but thats fine. We cache results for a short period
+        # of time to prevent hitting API limits. Note that forecast.io will
+        # charge users for too many calls in 1 day, so take care when updating.
+        self.forecast_data.update()


Is this call still needed if we hit minutely_summary as that involves it's own update call?

Looking at the SDK we call down into, calling update makes an API call to forecast.io which gets us everything we need. We then call update on each individual component. With those calls, the object we get back from SDK has a method to see if the data (minutely, hourly, etc.) already exists in the json object it got back from the API. If it doesn't, then it'll make a new call to forecast.io. Looking at it though, that new call is hitting the same endpoint for the same data, so for our simple implementation, there is no reason it wouldn't already have all the data it needs.

Those methods also have rate limits on them from our side of things. (2 minutes) Since the update all call occurs first and we cache the result for 2 minutes, it's probably unnecessary to throttle all of those update calls, but a nice safeguard in my opinion.

oudeismetis · 2016-06-13T22:45:01Z

tests/components/test_forecast.py

-        with pytest.raises(HTTPError):
-            forecast.setup_platform(self.hass, self.config, MagicMock())
+        response = forecast.setup_platform(self.hass, self.config, MagicMock())
+        self.assertFalse(response)


Not sure what is convention for hass. I wrapper the portion of the setup that calls forecast.io in a try/catch. In the catch, it logs an error message and just returns false. I've seen some components do that. Others return None or [ ]

Could just as easily raise an error.

oudeismetis · 2016-06-13T23:45:10Z

@balloob rebased and fixed tests.
Let me know if you see anything else of interest.

balloob · 2016-06-14T01:54:11Z

Looks good! 🐬

rmkraus reviewed Jun 3, 2016
View reviewed changes

fabaff reviewed Jun 3, 2016
View reviewed changes

robbiet480 mentioned this pull request Jun 5, 2016

Add tests for forecast.io #2227

Merged

oudeismetis reviewed Jun 11, 2016
View reviewed changes

kellerza reviewed Jun 11, 2016
View reviewed changes

balloob reviewed Jun 13, 2016
View reviewed changes

oudeismetis added 6 commits June 13, 2016 09:29

Refactor Forecast.io

a868be4

Some more refactoring and code review workoff

df3d80b

Dict switch refactor

3e5990f

CamelCase for data lookup

7ebf1be

Fixing unit_of_measure update

35dc4a7

Better default return for unit_of_measurement

f784073

oudeismetis force-pushed the refactor-forecastio branch from 9cc87cb to f784073 Compare June 13, 2016 13:39

Test fix

64e0c6e

oudeismetis reviewed Jun 13, 2016
View reviewed changes

balloob merged commit 8e839be into home-assistant:dev Jun 14, 2016

home-assistant locked and limited conversation to collaborators Mar 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Forecast.io #2217

Refactor Forecast.io #2217

oudeismetis commented Jun 2, 2016 •

edited

Loading

rmkraus Jun 3, 2016

oudeismetis Jun 3, 2016

rmkraus commented Jun 3, 2016

oudeismetis commented Jun 3, 2016

fabaff Jun 3, 2016

oudeismetis Jun 3, 2016

oudeismetis commented Jun 7, 2016

kellerza commented Jun 11, 2016

oudeismetis commented Jun 11, 2016

oudeismetis Jun 11, 2016

kellerza Jun 11, 2016

MartinHjelmare Jun 12, 2016 •

edited

Loading

MartinHjelmare Jun 12, 2016

oudeismetis Jun 12, 2016

kellerza commented Jun 11, 2016

kellerza Jun 11, 2016

oudeismetis Jun 12, 2016

kellerza Jun 12, 2016

oudeismetis Jun 12, 2016

oudeismetis Jun 12, 2016

balloob commented Jun 13, 2016

balloob Jun 13, 2016

oudeismetis Jun 13, 2016

oudeismetis Jun 13, 2016

oudeismetis commented Jun 13, 2016

balloob commented Jun 14, 2016

Refactor Forecast.io #2217

Refactor Forecast.io #2217

Conversation

oudeismetis commented Jun 2, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmkraus commented Jun 3, 2016

oudeismetis commented Jun 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oudeismetis commented Jun 7, 2016

kellerza commented Jun 11, 2016

oudeismetis commented Jun 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartinHjelmare Jun 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kellerza commented Jun 11, 2016

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

balloob commented Jun 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oudeismetis commented Jun 13, 2016

balloob commented Jun 14, 2016

oudeismetis commented Jun 2, 2016 •

edited

Loading

MartinHjelmare Jun 12, 2016 •

edited

Loading