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

Fix forecast display problem in Firefox #12073

Merged
merged 1 commit into from Jan 31, 2018

Conversation

escoand
Copy link
Contributor

@escoand escoand commented Jan 30, 2018

Description:

In Firefox the forecast chart is empty because of a not working timestamp format.

Related issue (if applicable): fixes #12055

Example entry for configuration.yaml (if applicable):

weather:
  - platform: openweathermap
    api_key: !secret owm_api

Checklist:

  • The code change is tested and works locally.

If the code does not interact with devices:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • Tests have been added to verify that the new code works.

@homeassistant
Copy link
Contributor

Hi @escoand,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@homeassistant homeassistant added cla-needed merging-to-master This PR is merging into the RC branch and should probably change the branch to `dev`. platform: weather.openweathermap cla-signed and removed cla-needed labels Jan 30, 2018
@fabaff fabaff changed the base branch from master to dev January 30, 2018 22:06
@fabaff fabaff changed the base branch from dev to master January 30, 2018 22:07
@fabaff
Copy link
Member

fabaff commented Jan 30, 2018

PRs can only be merged to dev.

@balloob balloob changed the base branch from master to dev January 30, 2018 22:28
@balloob
Copy link
Member

balloob commented Jan 30, 2018

It seems like you have some commits in this branch that are not yours, please clean it up.

})
if (len(data) - 1) % MIN_OFFSET_BETWEEN_FORECAST_CONDITIONS == 0:
data[len(data) - 1][ATTR_FORECAST_CONDITION] = \
[k for k, v in CONDITION_CLASSES.items() if entry.get_weather_code() in v][0]

Choose a reason for hiding this comment

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

line too long (96 > 79 characters)

ATTR_FORECAST_TEMP: entry.get_temperature('celsius').get('temp')
})
if (len(data) - 1) % MIN_OFFSET_BETWEEN_FORECAST_CONDITIONS == 0:
data[len(data) - 1][ATTR_FORECAST_CONDITION] = \

Choose a reason for hiding this comment

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

indentation is not a multiple of four

for entry in self.forecast_data.get_weathers():
data.append({
ATTR_FORECAST_TIME: entry.get_reference_time('iso'),
ATTR_FORECAST_TEMP: entry.get_temperature('celsius').get('temp')

Choose a reason for hiding this comment

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

line too long (80 > 79 characters)

@escoand
Copy link
Contributor Author

escoand commented Jan 30, 2018

Recreated PR branch on dev.

@Danielhiversen Danielhiversen removed the merging-to-master This PR is merging into the RC branch and should probably change the branch to `dev`. label Jan 31, 2018
@fabaff fabaff changed the title weather.openweathermap: fix forecast display problem in firefox Fix forecast display problem in Firefox Jan 31, 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.

Thanks 🐦

@fabaff fabaff merged commit 4cb1f93 into home-assistant:dev Jan 31, 2018
@escoand escoand deleted the owm_forecast_timestamp branch January 31, 2018 11:20
@balloob balloob mentioned this pull request Feb 9, 2018
@home-assistant home-assistant locked and limited conversation to collaborators May 29, 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.

Wrong timestamp in forecast
6 participants