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 sensor : current weather and 1 hour rain forecast #17773

Merged
merged 21 commits into from Oct 29, 2018

Conversation

victorcerutti
Copy link
Contributor

@victorcerutti victorcerutti commented Oct 24, 2018

Description:

Météo France provides a convenient rain forecast that is pretty accurate.
This sensor gives the time to next rain forecasted.
It also display several sensor for monitored conditions, as in the example
@MartinHjelmare @fabaff : I screwed up my last PR during a rebase. I took account of your requests and this is a new release of the Météo-France platform, using a pypi package

Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#<home-assistant.io PR number goes here>
home-assistant/home-assistant.io#6938

Example entry for configuration.yaml

sensor:
  - platform: meteo_france
    postal_code: 76000
    monitored_conditions:
      - rain_chance
      - freeze_chance
      - thunder_chance
      - snow_chance
      - weather
      - next_rain
      - wind_speed
      - temperature
      - uv

Checklist:

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

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
Copy link
Member

fabaff commented Oct 24, 2018

Successor of #17602.

Support for Meteo France raining forecast.

For more details about this platform, please refer to the documentation at
https://home-assistant.io/components/sensor.meteofrance/
Copy link
Member

Choose a reason for hiding this comment

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

There's a missing underscore in the page name.


PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend({
vol.Required(CONF_POSTAL_CODE): cv.string,
vol.Required(CONF_MONITORED_CONDITIONS, default=[]):
Copy link
Member

Choose a reason for hiding this comment

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

Default isn't necessary if key is required.

})


def setup_platform(hass, config, add_devices, discovery_info=None):
Copy link
Member

Choose a reason for hiding this comment

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

Rename add_devices to add_entities.


def setup_platform(hass, config, add_devices, discovery_info=None):
"""Set up the Meteo-France sensor."""
postal_code = config.get(CONF_POSTAL_CODE)
Copy link
Member

Choose a reason for hiding this comment

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

Use dict[key] for required config keys.

@property
def name(self):
"""Return the name of the sensor."""
return self._data["name"]+' '+SENSOR_TYPES[self._condition][0]
Copy link
Member

Choose a reason for hiding this comment

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

Use new style string formatting instead of string concatenation.

"""Return the state of the sensor."""
if self._data[self._condition] is not False:
return self._data[self._condition]
return STATE_UNKNOWN
Copy link
Member

Choose a reason for hiding this comment

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

Use None to represent unknown state.

return STATE_UNKNOWN

@property
def state_attributes(self):
Copy link
Member

Choose a reason for hiding this comment

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

Replace this with device_state_attributes.


@property
def state_attributes(self):
"""Return the state attributes of the sun."""
Copy link
Member

Choose a reason for hiding this comment

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

Stale docstring.

}
}
except KeyError:
_LOGGER.error("No result for the monitored condition `{}`".format(self._condition))

Choose a reason for hiding this comment

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

line too long (99 > 79 characters)

try:
return self._data[self._condition]
except KeyError:
_LOGGER.error("No result for the monitored condition `{}`".format(self._condition))

Choose a reason for hiding this comment

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

line too long (95 > 79 characters)

try:
return self._data[self._condition]
except KeyError:
_LOGGER.error("No result for the condition `%s`", self._condition)
Copy link
Member

Choose a reason for hiding this comment

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

Please don't log in entity properties. We don't want any side effects here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review.
Some cities doesn't have all the conditions returned by the package. What should I do about the sensor then ?
Without the exception catching, it fails, generates an error and the sensor is not displayed on the front-end.
I can also keep the exception catching without logging. Sensors stay on the front-end with a None value and it's up to the user to remove the monitored-condition that is not available for this location. But without error logging, there is not hint that it's the problem

Maybe another solution would be to still catch those KeyError and set the state as "no data available for this location".

Copy link
Member

Choose a reason for hiding this comment

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

Just move the access and exception handling etc to update and save the state in an instance attribute. Return the instance attribute here in the state property.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

latest commit should fix this

}
}
except KeyError:
_LOGGER.error("No result for the condition `%s`",
Copy link
Member

Choose a reason for hiding this comment

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

See above.

Copy link
Member

Choose a reason for hiding this comment

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

This isn't resolved.

Errors are now catched at sensor initialization and state is set when updating the data
self._data = client.get_data()
self._state = None
try:
self._state = self._data[self._condition]
Copy link
Member

Choose a reason for hiding this comment

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

Instead call add_entities with second argument True. It will make sure that update is called during entity addition.

We don't want side effects in init either.

"""Initialize the sensor."""
self._condition = condition
self._client = client
self._data = client.get_data()
Copy link
Member

Choose a reason for hiding this comment

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

Why are we only calling this here, one time? Shouldn't this be called on every update?

We should probably move this to update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm new to Python but here is my understanding :
As client.get_data returns a dict, it passes the reference to the client data
When it's updated after the remote fetch/parsing of the package, the sensor _data is up to date.

I updated to call get_data() after the update to be sure the reference is never broken for whatever reason may happen

Copy link

@houndci-bot houndci-bot left a comment

Choose a reason for hiding this comment

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

Some files could not be reviewed due to errors:

Traceback (most recent call last):
Traceback (most recent call last):
  File "/home/linters/.local/bin/flake8", line 7, in 
    from flake8.main.cli import main
ModuleNotFoundError: No module named 'flake8'

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 good!

@MartinHjelmare
Copy link
Member

@fabaff ok with you?

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.

Fine with me 🐦

@fabaff fabaff merged commit cb73a8b into home-assistant:dev Oct 29, 2018
@ghost ghost removed the in progress label Oct 29, 2018
@balloob balloob mentioned this pull request Nov 9, 2018
@romquenin
Copy link

Hello, I would love to use this sensor for it's french human-readable text summary of the current conditions but unfortunately my foreign city is not available. Could it be possible to add a european city with it postal code (previsions-meteo-monde/londres/03772) 03772 for london.
It would be a great feature for French People living abroad.
Thanks.

@MartinHjelmare
Copy link
Member

If you want to suggest an enhancement please open a feature request in the Feature Requests section of our community forum.

Merged PRs should not be used for enhancement discussion.

Thanks!

@home-assistant home-assistant locked as resolved and limited conversation to collaborators Jan 10, 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