Skip to content

Forecasts for weather underground#7062

Merged
balloob merged 1 commit into
home-assistant:devfrom
pezinek:wu_forecasts
May 6, 2017
Merged

Forecasts for weather underground#7062
balloob merged 1 commit into
home-assistant:devfrom
pezinek:wu_forecasts

Conversation

@pezinek
Copy link
Copy Markdown
Contributor

@pezinek pezinek commented Apr 11, 2017

Description:

This PR allows forecasts for the Weather Underground component.

Related issue (if applicable): N/A

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

Example entry for configuration.yaml (if applicable):

  sensor:
  - platform: wunderground
    api_key: your_api_key
    monitored_conditions:
      - weather_1d_metric

Checklist:

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

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.

@mention-bot
Copy link
Copy Markdown

@pezinek, thanks for your PR! By analyzing the history of the files in this pull request, we identified @tchellomello, @arsaboo and @Teagan42 to be potential reviewers.


MIN_TIME_BETWEEN_UPDATES_ALERTS = timedelta(minutes=15)
MIN_TIME_BETWEEN_UPDATES_OBSERVATION = timedelta(minutes=5)
MIN_TIME_BETWEEN_UPDATES = timedelta(minutes=5)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pezinek with this change, are you considering the threshold of 500 calls a day?

Copy link
Copy Markdown
Contributor Author

@pezinek pezinek Apr 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, in fact with this change all the different API requests (e.g. alerts, conditions, forecasts,... etc.) are combined into single one according to these instructions: https://www.wunderground.com/weather/api/d/docs?d=data/index#examples
So in fact this component will not do more than 288 API calls per day (for single instance)

(FIY: I'm running this code for about a 2 weeks on my home instance already, and I didn't hit the API limit)

@tchellomello are you OK with my explanation above or are there additional steps I need to do before this PR could be approved ?

@rpitera
Copy link
Copy Markdown

rpitera commented Apr 22, 2017

Bummed this didn't make it into the .43 release... maybe in a point release??

@pezinek
Copy link
Copy Markdown
Contributor Author

pezinek commented May 2, 2017

Not sure what should be the next step for this PR, but please let me know if there is still something I am suposed to do in order to proceed further with this request.

Copy link
Copy Markdown
Contributor

@Teagan42 Teagan42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why you have this WUSensorConfig when almost all of those fields are available/overrideable in the Entity class


# Helper classes for declaring sensor configurations

class WUSensorConfig(object):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't you just use the entity class for these?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Teagan42, thanks for checking the code !!!
The reasons why I decided to have separate classes for storing the configuration is that the WU API is a mess and I didn't wanted to clutter the WUndergroundSensor code by all the different parsing methods for different sensor types. The second reason to have the parsing/configuration for the sensors in separate class is to allow the possibility for having multiple weather stations in the future, where multiple sensors for the same thing (with the very same configuration/parsing) will coexist in parallel, and instantiating the Entity subclasses directly in the SENSOR_TYPES list will make this much more difficult.

@balloob
Copy link
Copy Markdown
Member

balloob commented May 5, 2017

Okay, sorry that it took so long for anyone to take a look at your PR. This is great and I'll be down to merge it.

However, do note that we're in the process of moving all weather related sensors to the weather component with the goal that we get a nice UI in the frontend.

So it's up to you, merge this as is or migrate it to the weather component (should be pretty straight forward)

Copy link
Copy Markdown
Member

@balloob balloob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐬

@balloob balloob added this to the 0.44 milestone May 5, 2017
@pezinek
Copy link
Copy Markdown
Contributor Author

pezinek commented May 5, 2017

Actually the weather component right now does have limited set of monitored conditions, so I guess it may have sense to merge this as is and later on write another separate WU component that will plug in to the new weather architecture, similarly as openweathermap components do.

@rpitera
Copy link
Copy Markdown

rpitera commented May 6, 2017

Sooo..... will this make 0.44? Asking for a friend... 😉

@balloob balloob merged commit 7a70496 into home-assistant:dev May 6, 2017
@rpitera
Copy link
Copy Markdown

rpitera commented May 6, 2017

Thanks very much!!

@balloob
Copy link
Copy Markdown
Member

balloob commented May 7, 2017

Will be included in 0.44.1 . Forgot about it for 0.44.

@balloob balloob modified the milestones: 0.44.1, 0.44 May 7, 2017
@rpitera
Copy link
Copy Markdown

rpitera commented May 7, 2017

No problem, man. 0.44 had a lot going on!

balloob pushed a commit that referenced this pull request May 7, 2017
@balloob
Copy link
Copy Markdown
Member

balloob commented May 7, 2017

Cherry-picked for 0.44.1

@balloob balloob mentioned this pull request May 7, 2017
@home-assistant home-assistant locked and limited conversation to collaborators Aug 12, 2017
@pezinek pezinek deleted the wu_forecasts branch October 23, 2017 22:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants