Skip to content

Conversation

@marcelstoer
Copy link
Member

@marcelstoer marcelstoer requested a review from squix78 July 10, 2018 20:58
@squix78
Copy link
Collaborator

squix78 commented Jul 10, 2018

Thanks for the PR. I would have suggested to add a a second update method and leave it to the users which one to call. The City,Country method is very easy if not perfect for beginners, while the one by ID is perfect but potentially intimidating for beginners. Since this is for educational purposes we should offer both. What do you think?

@marcelstoer marcelstoer mentioned this pull request Jul 11, 2018
Closed
@marcelstoer
Copy link
Member Author

marcelstoer commented Jul 11, 2018

Alright, will do as follows:

  • new functions updateCurrentById, updateForecastsById to avoid introducing breaking changes
  • extract the URL-building part into function to be reused by both before calling doUpdate
  • demos will use the new functions
  • add comment in settings.h as for how to obtain the right location id
  • once released I'll update the other clients accordingly

@marcelstoer marcelstoer merged commit 758f05e into master Jul 26, 2018
@marcelstoer marcelstoer deleted the fix/owm-location-id branch October 19, 2018 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants