Skip to content

Add precipitation to OpenWeatherMap forecast#13971

Merged
fabaff merged 10 commits intohome-assistant:devfrom
escoand:add-precipitation
Apr 29, 2018
Merged

Add precipitation to OpenWeatherMap forecast#13971
fabaff merged 10 commits intohome-assistant:devfrom
escoand:add-precipitation

Conversation

@escoand
Copy link
Copy Markdown
Contributor

@escoand escoand commented Apr 17, 2018

Description:

Add the precipitation into the forecast of OpenWeatherMap.
The corresponding pull-request for the weather card is: home-assistant/frontend#1098

Example entry for configuration.yaml (if applicable):

weather:
  - platform: openweathermap
    api_key: !secret key_owm

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

Copy link
Copy Markdown
Member

@pvizeli pvizeli left a comment

Choose a reason for hiding this comment

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

You need do that also on component level. Update demo platform and write tests.

@pvizeli pvizeli requested a review from fabaff April 18, 2018 10:41
@escoand
Copy link
Copy Markdown
Contributor Author

escoand commented Apr 18, 2018

You're right but there are some more things need to be fixed.
I used temperature_low from other platforms as template. They are implemented in a similar way. Should they be fixed in an additional PR?

return

self.forecast_data = fcd.get_forecast()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

blank line at end of file

assert data.get(ATTR_FORECAST)[0].get(ATTR_FORECAST_TEMP) == 22
assert data.get(ATTR_FORECAST)[0].get(ATTR_FORECAST_TEMP_LOW) == 15
assert data.get(ATTR_FORECAST)[6].get(ATTR_FORECAST_CONDITION) == 'fog'
assert data.get(ATTR_FORECAST)[6].get(ATTR_FORECAST_PRECIPITATION) == 0.2
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

line too long (81 > 79 characters)

assert data.get(ATTR_WEATHER_OZONE) is None
assert data.get(ATTR_WEATHER_ATTRIBUTION) == \
'Powered by Home Assistant'
assert data.get(ATTR_FORECAST)[0].get(ATTR_FORECAST_CONDITION) == 'rainy'
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

line too long (81 > 79 characters)

@escoand
Copy link
Copy Markdown
Contributor Author

escoand commented Apr 18, 2018

@pvizeli Is this like you expected?

Copy link
Copy Markdown
Member

@syssi syssi left a comment

Choose a reason for hiding this comment

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

LGTM.

@escoand
Copy link
Copy Markdown
Contributor Author

escoand commented Apr 22, 2018

More complaints?

@escoand
Copy link
Copy Markdown
Contributor Author

escoand commented Apr 29, 2018

Are there any more problems prevent merging?

@syssi
Copy link
Copy Markdown
Member

syssi commented Apr 29, 2018

Could you resolve the merge conflict?

@escoand
Copy link
Copy Markdown
Contributor Author

escoand commented Apr 29, 2018

@syssi Done!

@syssi
Copy link
Copy Markdown
Member

syssi commented Apr 29, 2018

@fabaff Could you take a look at this PR? Thanks! :-)

Copy link
Copy Markdown
Member

@fabaff fabaff left a comment

Choose a reason for hiding this comment

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

Thanks 🐦

@fabaff fabaff merged commit 8e7f500 into home-assistant:dev Apr 29, 2018
@balloob balloob mentioned this pull request May 11, 2018
@home-assistant home-assistant locked and limited conversation to collaborators Sep 5, 2018
@escoand escoand deleted the add-precipitation branch September 23, 2019 21:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants