-
-
Notifications
You must be signed in to change notification settings - Fork 314
Make Population a Property of Location Object #188
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
Conversation
|
Nice work. Just throwing this out there: would it perhaps be better to name the property Also, I think you should move the |
|
Should be good to go! I can't test everything locally (pipenv's being annoying), so if you might want to verify that everything works as intended before merging |
|
Thanks for this! I'll look over it and merge. |
…county_code is specified.
|
Is this ready to merge or are there more changes that I should make? |
|
@Turreted hey, while testing, your initial data-source was missing populations for a bunch of countries, so I decided to look for another one. I opted with https://www.geonames.org/ and they seem to provide relatively updated populations as well as for every country we currently have. The populations are now also fetched when starting the app. There's no point of having a refresh cache mechanism on it as it probably doesn't update very often so, so it's fine to just retrieve the data on startup instead. Looks good to go now. |
| return mappings | ||
|
|
||
| # Mapping of alpha-2 codes country codes to population. | ||
| populations = fetch_populations() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could make this a "start_up event"
https://fastapi.tiangolo.com/advanced/events/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, it seems to work on startup now though? @Kilo59
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely, what you have here looks like it would work fine. Just pointing out a feature of FastAPI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Kilo59 alright :) We can look at it later. Now populations should work all perfectly. That's a nice feature.
|
@all-contributors please add @Turreted for code. |
|
I've put up a pull request to add @Turreted! 🎉 |
Note that this only works for countries, and will return None is a province is specified. Also note that this does not add anything to that API, It simply extends the location object.
All data is pulled from the World Bank, and can be found Here