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

initial draft of batch geocoding #1046

Closed
wants to merge 4 commits into from
Closed

initial draft of batch geocoding #1046

wants to merge 4 commits into from

Conversation

aaronpk
Copy link
Contributor

@aaronpk aaronpk commented May 18, 2016

  • Adds new Geocoder.batch method which takes an array of input text and uses the provider's batch API to make a single HTTP request to geocode all the inputs.
  • Updates readme with a usage example.

* Adds new `Geocoder.batch` method which takes an array of input text and uses the provider's batch API to make a single HTTP request to geocode all the inputs.
* Updates readme with a usage example.
@aaronpk
Copy link
Contributor Author

aaronpk commented May 18, 2016

for #1044

@aaronpk
Copy link
Contributor Author

aaronpk commented May 18, 2016

I've started a draft of how this might work. I'd appreciate a quick review before I go any farther with it. I want to make sure I'm not missing any assumptions. I've only added batch support to the Esri provider, since I'm not sure if other providers have a batch API. The Esri service requires authentication in order to use the batch API, so if you want to run it yourself you'll need to register for a developer account at https://developers.arcgis.com

since the Esri batch geocoder can generate requests that are potentially very large, it's better to set the request method to POST for those. this adds a configuration option so GET/POST can be used on any provider.
@TrangPham
Copy link
Contributor

It looks good so far! :)

Could more of the functionality for batch be handled in the base lookup?
Ideally implementing batch for a lookup should be simple and easy to understand.
@alexreisner Do you have suggestions?

@alexreisner
Copy link
Owner

alexreisner commented Jun 7, 2016

I actually like the separation of batch and single requests. In fact, I wonder whether we shouldn't go a step further and use entirely separate lookups for batch requests. Reasons include: (1) combining them is potentially misleading, since only one lookup has batch capability, (2) single and batch queries are used in totally different settings, and (3) there's no reason to expect future batch APIs to be as similar to their single-query counterparts as ESRI's are. Conversely to (3), they could be even more similar than ESRI's, so that argument works both ways.

The lack of a second batch API is what causes the confusion in (3) and makes this whole thing a little difficult to think about. I want to choose an architecture here that minimizes the chances of difficult integrations in the future. We should really look at how other batch APIs are implemented. I believe Bing and Mapquest provided batch lookups at one time but I'm not sure whether they still do.

As for the code, I have the following thoughts:

  • as discussed in add configuration parameter to send a POST request #1058, don't add the use_post config option (should be automatic); create_http_request should take a verb argument
  • the new class should be called BatchQuery, for clarity
  • BatchQuery should probably inherit from Query, since it has a similar interface
    • we want the url method, esp for testing (provided by Query)
    • shouldn't copy/paste lookup method
    • reverse_geocode? should have a real implementation
  • remove debugging line (jj doc)
  • be careful of mixing in unrelated refactoring: I don't love the changes in Result::ESRI, which make it less explicit when the different hash keys will be used

@zhaomengru2015
Copy link

any updates on this PR? I am currently working on a project which is using sensis service, I am trying to implement a PR about sensis support, which will use the 'post' http request.

@alexreisner
Copy link
Owner

Closing this due to lack of activity, and the fact that creating a unified interface for bulk geocoding is quite difficult given the wide range of services offered (see comment above).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants