GTC-3081: Add political/id-lookup endpoint #616
Conversation
…pecifying admin level hierarchy
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## develop #616 +/- ##
===========================================
- Coverage 80.67% 79.91% -0.77%
===========================================
Files 131 133 +2
Lines 6014 6129 +115
===========================================
+ Hits 4852 4898 +46
- Misses 1162 1231 +69
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
…ally) unaccented ones
dmannarino
changed the title
app/routes/thematic/geoencoder.py
Outdated
| @@ -0,0 +1,216 @@ | |||
| import re | |||
There was a problem hiding this comment.
@dmannarino I know you said you didn't like thematic. I was thinking of other ideas, is this admin area specific endpoints, maybe we just put it under like something like "political"? E.g. /political/geoencoder. Then the future can include additional GADM endpoints, WDPA, concessions, etc. Not sure if we need to distinguish it from the rest of the API, or we can just have it all in under a header in the docs.
There was a problem hiding this comment.
But I do think geoencoder is a little vague if it only works for admin areas, geoencoding implies converting any text of the place into coordinates: https://en.wikipedia.org/wiki/Address_geocoding
There was a problem hiding this comment.
Changed to /political/id-lookup
Thanks! |
dmannarino
changed the title
app/routes/political/id_lookup.py
Outdated
| ) | ||
|
|
||
|
|
||
| def sanitize_names( |
There was a problem hiding this comment.
What about normalize_names? Sanitize comes with a different connotation about what this method does (for me, anyway).
There was a problem hiding this comment.
Roger, will change, thanks!
Yes, that's the right thing to do. :) Will do, thanks! |
app/routes/political/id_lookup.py
Outdated
| "adminSource": admin_source, | ||
| "adminVersion": admin_version, | ||
| "matches": matches, | ||
| } |
There was a problem hiding this comment.
Nit (optional): I guess this code works in Pydantic, but the types would be clearer if 'data' was a AdminIDLookupResponseData, which I guess you could do via:
data = AdminIDLookupResponseData(**{
"adminSource": admin_source,
"adminVersion": admin_version,
"matches": matches,
})
There was a problem hiding this comment.
Sounds good, will implement, thanks!
Pull request checklist
Please check if your PR fulfills the following requirements:
Pull request type
Please check the type of change your PR introduces:
What is the current behavior?
No geoencode endpoint
Issue Number: GTC-3081
What is the new behavior?
Does this introduce a breaking change?
Other information
Open to suggestions for a better URL path. Not really crazy about either "thematic" or "geoencode".