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

Added temperature (apparent) high/low, deprecated max/min #12233

Merged
merged 4 commits into from
May 15, 2018

Conversation

nordlead2005
Copy link
Contributor

@nordlead2005 nordlead2005 commented Feb 7, 2018

Description:

Dark Sky API has deprecated temperatureMin, temperatureMax, apparentTemperatureMin, and apparentTemperatureMax. I added 4 new temperature fields that are the replacements (high/low) and added a check for the old values and log a warning message that they are deprecated.

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

Example entry for configuration.yaml (if applicable):

sensor:
  - platform: darksky
    api_key: YOUR_API_KEY
    monitored_conditions:
      - summary
      - temperature_low
      - temperature_high

Checklist:

  • The code change is tested and works locally.

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

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

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

@@ -166,6 +182,9 @@ def setup_platform(hass, config, add_devices, discovery_info=None):
forecast = config.get(CONF_FORECAST)
sensors = []
for variable in config[CONF_MONITORED_CONDITIONS]:
if variable in DEPRECATED_SENSOR_TYPES:
_LOGGER.warning("Monitored condition %s is deprecated.",
Copy link
Member

Choose a reason for hiding this comment

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

Just a suggestion, but you could change DEPRECATED_SENSOR_TYPES to be dictionary instead of a set and provide the "new" name for the condition (maybe even with a message that their new "interval" like currently or hourly won't be supported anymore).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

currently and hourly don't seem to ever have been supported. I'm not sure why they were in the code. A dictionary isn't a bad idea though.

@fabaff
Copy link
Member

fabaff commented Feb 23, 2018

We don't interact with the Dark Sky API directly, we use python-forecastio. All API changes need to be addressed in python-forecastio. I assume that if you add temperature_low and temperature_high they will stay always empty because the data is not available.

@nordlead2005
Copy link
Contributor Author

nordlead2005 commented Feb 23, 2018

python-forecastio basically fetches a json object, provides easy access to various parts of the json object (daily, hourly, etc...) and you query it with whatever key you want. If Darksky adds or removes a json field to the daily data then python-forecastio needs no adjustments.

So, should we not notify the users of HA that _min and _max are deprecated? Eventually they may disappear with no warning and no required update by python-forecastio.

the new temperature_high and temperature_low fields work fine (ignoring the bug I had in the code I submitted, the future days worked, just not day 0).
image

@frenck
Copy link
Member

frenck commented Mar 13, 2018

Note, this changes the naming of the monitored conditions, so IMHO it is a breaking change as well.

@balloob
Copy link
Member

balloob commented May 2, 2018

This change seems fine. Marked it as a breaking change. It technically is not but it will be good to include why some types have been marked as deprecated and what the alternative is.

Ok to merge when merge conflict resolved.

@syssi syssi merged commit 852ce9f into home-assistant:dev May 15, 2018
@balloob balloob mentioned this pull request May 28, 2018
@home-assistant home-assistant locked and limited conversation to collaborators Sep 5, 2018
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.

7 participants