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

Add timezone property to city #140

Merged
merged 3 commits into from
Jan 22, 2020
Merged

Add timezone property to city #140

merged 3 commits into from
Jan 22, 2020

Conversation

paulhennell
Copy link

This adds support to extract the 'timezone' property now returned from the openweather API:
https://openweather.co.uk/blog/post/we-have-integrated-time-zones-our-weather-api-products

Includes

  • Phpdoc following existing style
  • Works with XML & Json responses
  • Passes existing tests (see notes)

Notes:

  • The fake data files had to be updated with timezone keys for existing tests to pass. It currently would fail should 'timezone' not exist in a response, but it doesn't look like other properties are checked for existence either so I've ignored that on the basis all responses appear to have a timezone now.

  • I've hard coded the 'population' constructor argument to null as I didn't want to remove it, but the population doesn't appear to exist in any responses to pass it through (it seems to be currently skipped by all construction). If population is never used it might be neater to remove it and put timezone in its place, but that feels like a potentially separate discussion / fix, albiet one that might be easier to do before adding this...

@cmfcmf
Copy link
Owner

cmfcmf commented Jan 1, 2020

Hi @hennell-git,

Thank you very much for your nicely done PR! I've adjusted it slightly in 98b173f to transform the UTC offsets into \DateTimeZones. That was more difficult than I had anticipated, but I got it working after a bit of fiddling around. Let me know if that works for you 👍

  • The fake data files had to be updated with timezone keys for existing tests to pass. It currently would fail should 'timezone' not exist in a response, but it doesn't look like other properties are checked for existence either so I've ignored that on the basis all responses appear to have a timezone now.

I think that's fine.

  • I've hard coded the 'population' constructor argument to null as I didn't want to remove it, but the population doesn't appear to exist in any responses to pass it through (it seems to be currently skipped by all construction). If population is never used it might be neater to remove it and put timezone in its place, but that feels like a potentially separate discussion / fix, albiet one that might be easier to do before adding this...

Good find! I think it is a remnant of when this library incorporated support for hourly or 16-day forecasts. I could find references to the population field over at

I added a note to #72 to remove it in the next major version.

Happy new year! 🍾

@cmfcmf cmfcmf merged commit 9c4df93 into cmfcmf:master Jan 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants