Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Météo-France platform for the weather component #18404

Merged
merged 19 commits into from Feb 14, 2019

Conversation

victorcerutti
Copy link
Contributor

@victorcerutti victorcerutti commented Nov 12, 2018

This is reopening of #18322 after I removed the weather model change I made.
Discussions about enhancing the weather component (model and frontend) have been posted to :

Description:

After the recently merged Météo-France sensor providing weather datas from Météo-France, here is the weather component
The configuration is similar to the weather sensor and the weather platform also provides a 4 days forecast.

Related issue (if applicable): fixes #

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

Example entry for configuration.yaml (if applicable):

weather:
  - platform: meteo_france
    city: '76000'

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.

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

If the code communicates with devices, web services, or third-party tools:

  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

fabaff
fabaff previously requested changes Nov 12, 2018
Copy link
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.

At first glance looks the rest pretty solid.

homeassistant/components/weather/meteo_france.py Outdated Show resolved Hide resolved
@victorcerutti
Copy link
Contributor Author

Please consider my last commit as a work in progress.
I wanted to check if I'm doing this correctly
This is a breaking change for the sensor as the configuration is now done this way:

meteo_france:
  - postal_code: 29000
    monitored_conditions:
        - rain_chance
        - freeze_chance
        - thunder_chance
        - snow_chance
        - weather
        - next_rain
        - wind_speed
        - temperature
        - uv
    weather_card: false
  - postal_code: 76000
    monitored_conditions:
      - temperature
  - postal_code: 51100

For each component, we display by default a weather card (unless config says otherwise) and a sensor for each monitored conditions (optional)

All the logic of client and update is managed by the meteo_france component.
Sensors and weather card only refers to a single client data fetcher for each city.

If this is correct, I will continue in this direction and update the documentation to reflect this changes

@victorcerutti
Copy link
Contributor Author

@fabaff Can you please review the way the config is set for this component ?
If this is ok I will continue and update the documentation and the PR to introduce this new component.

@victorcerutti
Copy link
Contributor Author

Hello,
I'd love to have feedback on this PR regarding the configuration here #18404 (comment)

Many thanks for considering my request !

@balloob
Copy link
Member

balloob commented Feb 6, 2019

We should not have a weather card config option, it's up to the user with Lovelace to decide if they want to show a weather card for an entity or not.

The config seems fine.

@victorcerutti
Copy link
Contributor Author

Thank you for the feedback
It makes total sense as Lovelace is now default to discard the weather_card option
I will correct this and update this PR soon

@victorcerutti
Copy link
Contributor Author

I think this is all good
I also updated the documentation according to the new configuration of the component

homeassistant/components/meteo_france.py Outdated Show resolved Hide resolved
homeassistant/components/meteo_france.py Outdated Show resolved Hide resolved
homeassistant/components/meteo_france.py Show resolved Hide resolved
homeassistant/components/meteo_france.py Outdated Show resolved Hide resolved
homeassistant/components/weather/meteo_france.py Outdated Show resolved Hide resolved
Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Looks great!

@MartinHjelmare
Copy link
Member

Can be merged when build passes.

@fabaff fabaff dismissed their stale review February 14, 2019 13:39

Comments addressed

@fabaff fabaff merged commit f4b2573 into home-assistant:dev Feb 14, 2019
@ghost ghost removed the in progress label Feb 14, 2019
@fabaff fabaff mentioned this pull request Feb 14, 2019
7 tasks
@anthosz
Copy link

anthosz commented Feb 21, 2019

Dear @fabaff @mxworm ,

Do you know in which version this component will be available?

Best regards,

@MartinHjelmare
Copy link
Member

0.89

@home-assistant home-assistant locked as resolved and limited conversation to collaborators Feb 21, 2019
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.

None yet

7 participants