Added ForecastIO agent #160

Merged
merged 5 commits into from Mar 11, 2014

Projects

None yet

5 participants

@scavone

No description provided.

@cantino
Owner

Hey @scavone, thanks for contributing! Do you think it'd make sense to combine this with the existing WeatherAgent?

@scavone

Honestly....I hadn't though of combining it. Wouldn't take much though to combine. I'll send over a new pull request soon.

@scavone

Also, in case anyone wants to use this as a separate agent, I have a typo in the code on line 83..."tp_i" should read "to_i".

@albertsun

Would the existing weather agent poll both services then? Optionally be configured to use one or the other?

@scavone

Yes. I started rewriting the weather service last night which to allow the user to choose which service they wanted data from. That was the easy part.

I'm working now on standardizing the result set from both services so they have similar JSON paths for downstream agents to consume.

I'll also allow a choice to be made on whether you want future forecasts or current day to allow some greater flexibility.

@cantino
Owner

That makes a lot of sense.

@cantino
Owner

How's it going?

@scavone

Mapped all the ForecastIO results to the original JSON structure from WeatherUnderground. Had a few leftover values I was trying to figure out for placement but I think I might just add them to the end and call it a day.

After doing all of that, I started thinking about how much of a pain it would be to add additional weather API's in the future. Would it be easier to just return the JSON as intended by each service? It would be a lot simpler to just add a function for a new API to just use the location and API key variables currently being saved rather than restructuring JSON every time.

Either way, I'll try and get what I wrote already pushed out this weekend for you to review.

@coveralls

Coverage Status

Coverage decreased (-0.7%) when pulling 67d3a63 on scavone:master into b16f105 on cantino:master.

@scavone

I'm assuming I need to add the additional parameters (service, which_day) to the spec/fixtures/agents.yml in order to pass?

@cantino
Owner

Thanks @scavone!

Since people will already be using this agent, we don't want to break it underneath them. Let's provide defaults for service (wunderground) and which_day (1).

You could do this by defining methods like:

def service
  options["service"].presence || "wunderground"
end

def which_day
  (options["which_day"].presence || 1).to_i
end

Then in validate_options, call the methods and validate that they're valid.

@cantino cantino commented on an outdated diff Feb 28, 2014
app/models/agents/weather_agent.rb
}
end
def validate_options
- errors.add(:base, "location is required") unless options['location'].present? || options['zipcode'].present?
+ errors.add(:base, "service is required") unless options['service'].present?
+ errors.add(:base, "service must be set to 'forecastio' or 'wunderground'") unless ["forecastio", "wunderground"].include?(options['service'])
+ errors.add(:base, "location is required") unless options['location'].present?
errors.add(:base, "api_key is required") unless options['api_key'].present?
@cantino
cantino Feb 28, 2014

could be unless key_setup?

@cantino cantino commented on an outdated diff Feb 28, 2014
app/models/agents/weather_agent.rb
@@ -44,38 +52,120 @@ def working?
event_created_within?(2) && !recent_error_logs?
end
- def wunderground
- Wunderground.new(options['api_key']) if key_setup?
- end
-
def key_setup?
options['api_key'] && options['api_key'] != "your-key"
@cantino
cantino Feb 28, 2014

Maybe

   options['api_key'].present? && options['api_key'] != "your-key"
@cantino cantino commented on an outdated diff Feb 28, 2014
app/models/agents/weather_agent.rb
end
def check
if key_setup?
- wunderground.forecast_for(options['location'] || options['zipcode'])["forecast"]["simpleforecast"]["forecastday"].each do |day|
- if is_tomorrow?(day)
- create_event :payload => day.merge('location' => options['location'] || options['zipcode'])
- end
+ if options['service'] == 'forecastio'
+ weather = model(forecastio,options['service'],options['which_day'])
+ elsif options['service'] == 'wunderground'
+ weather = model(wunderground,options['service'],options['which_day'])
@cantino
cantino Feb 28, 2014

Style: We put spaces after commas in function calls, do it should be weather = model(wunderground, options['service'], options['which_day'])

@KenYN KenYN and 1 other commented on an outdated diff Feb 28, 2014
app/models/agents/weather_agent.rb
- errors.add(:base, "location is required") unless options['location'].present? || options['zipcode'].present?
- errors.add(:base, "api_key is required") unless options['api_key'].present?
+ errors.add(:base, "service is required") unless service.present?
+ errors.add(:base, "service must be set to 'forecastio' or 'wunderground'") unless ["forecastio", "wunderground"].include?(service)
+ errors.add(:base, "location is required") unless options['location'].present?
+ errors.add(:base, "api_key is required") unless key_setup?
+ errors.add(:base, "which_day selection is required") unless which_day.present?
+ end
+
+ def wunderground
+ data = Wunderground.new(options['api_key']).forecast_for(options['location'])['forecast']['simpleforecast']['forecastday'] if key_setup?
+ return data
+ end
+
+ def forecastio
+ ForecastIO.api_key = options['api_key'] if key_setup?
@KenYN
KenYN Feb 28, 2014

We don't test for key_setup? in wunderground. Also, if we are testing, this is better:

if key_setup?
  ForecastIO.api_key = options['api_key']
  lat, long = options['location'].split(',')
  data = ForecastIO.forecast(lat, long)['daily']['data']
  return data
end

The normal flow of code with validate_options ensures that key_setup? must be true here, but since it's a public method, perhaps we do need the guard?

BTW, is there a coding convention for return versus falling though?

@cantino
cantino Mar 1, 2014

I generally use return when I want to short circuit early, but not at the end of a method.

@coveralls

Coverage Status

Coverage decreased (-0.04%) when pulling 3a63cf5 on scavone:master into b16f105 on cantino:master.

@cantino
Owner

Nice work @scavone. I'm going to let this run for a couple of days on my Huginn instance, and if everything looks good, I think we can merge it!

@cantino
Owner

Ran into one error: we used to call the "location" option "zipcode", so my older agents broke. Perhaps you could allow both for backwards compatibility?

    def location
      options["location"].presence || options["zipcode"]
    end

    ...

    def validate_options
      errors.add(:base, "location is required") unless location.present?
      ..
    end

and then also forecast_for(location), lat, lng = location.split(','), etc.

@cantino cantino merged commit 49be934 into cantino:master Mar 11, 2014

1 check failed

Details default The Travis CI build failed
@cantino
Owner

Nice work @scavone, thank you!

@scavone

Definitely enjoy the app. Glad I could give a little back. Also, thanks for helping guide me through this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment