Add geocodio #468
Add geocodio #468
Conversation
|
@KostyaEsmukov Any feedback on this PR or thoughts on when it might be merged? Thanks! |
There was a problem hiding this comment.
Thanks! Sorry for the late response.
Overall looks good, I left a couple of minor comments.
The API returns a
422status code for invalid queries. This PR handles these using the_geocoder_exception_handlermethod, but it might be reasonable to map this code to aGeocoderQueryErroringeopy.geocoders.base.ERROR_CODE_MAP.
I guess it won't break anything if we move the 422 error to the ERROR_CODE_MAP. However, keeping that in the geocoder's own _geocoder_exception_handler method has a benefit of being more explicit, and it will protect against possibility of future refactorings breaking that error processing for Geocodio.
Pick whatever you like the most, I'm okay with both options.
| if street is not None and \ | ||
| not any(p is not None for p in (city, state, postal_code)): | ||
| raise GeocoderQueryError('If street is provided must also provide city, ' | ||
| 'state, and/or postal_code.') |
There was a problem hiding this comment.
Is there a reason why these conditions must be checked on the client-side?
(I'm okay with keeping these unless they might change in future).
There was a problem hiding this comment.
I think the only argument would be to save making an invalid request, which might count against rate limits or account request limits.
Okay, I'm a fan of being explicit, so I'll just stick with handling it in the |
Adds geocoder class for Geocod.io API.
The API returns a
422status code for invalid queries. This PR handles these using the_geocoder_exception_handlermethod, but it might be reasonable to map this code to aGeocoderQueryErroringeopy.geocoders.base.ERROR_CODE_MAP.#465