Weather Platform - IPMA#14716
Conversation
MartinHjelmare
left a comment
MartinHjelmare
left a comment
There was a problem hiding this comment.
Looks good. Didn't look at the tests yet. Some comments below.
| """ | ||
| import logging | ||
| from datetime import timedelta | ||
| import async_timeout |
There was a problem hiding this comment.
Group 3rd party imports and have a blank line between the three groups standard library, 3rd party and homeassistant imports.
|
|
||
| def __init__(self, station, config): | ||
| """Initialise the platform with a data instance and station name.""" | ||
| self._stationname = config.get(CONF_NAME, station.local) |
| @property | ||
| def condition(self): | ||
| """Return the current condition.""" | ||
| return [k for k, v in CONDITION_CLASSES.items() |
There was a problem hiding this comment.
return next((
k for k, v in CONDITION_CLASSES.items()
if self._forecast[0].idWeatherType in v), None)| @property | ||
| def visibility(self): | ||
| """Return the current visibility.""" | ||
| return None |
There was a problem hiding this comment.
Don't overwrite if not used.
| data_out = {} | ||
| data_out[ATTR_FORECAST_TIME] = data_in.forecastDate | ||
| data_out[ATTR_FORECAST_CONDITION] =\ | ||
| [k for k, v in CONDITION_CLASSES.items() |
There was a problem hiding this comment.
Use next and generator expression.
| return fcdata_out | ||
|
|
||
| @property | ||
| def state_attributes(self): |
There was a problem hiding this comment.
Don't overwrite state_attributes. Use device_state_attributes. It will be merged with state_attributes during a state update.
|
Thanks for the review! 👍 |
MartinHjelmare
left a comment
MartinHjelmare
left a comment
There was a problem hiding this comment.
Looks like it should be possible to mock the dependency and then we wouldn't need to add it to test requirements.
There's a helper MockDependency to mock a dependency in tests/common.py.
MartinHjelmare
left a comment
MartinHjelmare
left a comment
There was a problem hiding this comment.
Missed this one. Can be merged after that.
|
|
||
| if None in (latitude, longitude): | ||
| _LOGGER.error("Latitude or longitude not set in Home Assistant config") | ||
| return False |
|
Coveralls been acting up lately. Merging since we've got 97% coverage in this module. |
* initial commit * lint * update with pyipma * Added test * Added test * lint * missing dep * address comments * lint * make sure list is iterable * don't bother with list * mock dependency * no need to add test requirements * last correction
ghost
added
3 participants
Description:
This new platform provides weather information made publicly available by the Portuguese Weather service (Instituto Português do Mar e Atmosfera - IPMA)
Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#5465
Example entry for
configuration.yaml(if applicable):Checklist:
tox. Your PR cannot be merged unless tests passIf user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
REQUIREMENTSvariable (example).requirements_all.txtby runningscript/gen_requirements_all.py..coveragerc.If the code does not interact with devices: