Real batch geocoding for TomTom #325
Conversation
Fixing fetch adapter for non-json responses
Fix Here geocoding ApplicationError handling
|
@nchaulet I'm sure that looking into this it isn't among your top priorities, I'm wondering if you have had a chance to look at this because I'm planning to add this very same support for Here sooner than later and these changes depend on this PR. Thanks! |
|
Hi @charly22 I took a first look at this and it looks good to me, I still want to run it locally too, I have one suggestion that I would love you to address. |
| @@ -61,21 +61,32 @@ Geocoder.prototype.reverse = function(query, callback) { | |||
| * @return promise | |||
| */ | |||
| Geocoder.prototype.batchGeocode = function(values, callback) { | |||
| return BPromise.resolve(values) | |||
| return BPromise.resolve() | |||
There was a problem hiding this comment.
I think it will make sense to move that part to the abstract geocoder and to rely on the existence of _batchGeocoder instead of relying on throwing an error to have a fallback
There was a problem hiding this comment.
It does make sense, in fact, it was my first approach but I can't remember why I've fallen back to this way. I'll give it another try
What does this PR do?
Changes the strategy used to geocoding in batch using async batch TomTom API
Brief of changes
AbstractGeocodercalls_batchGeocodeof the provider who has this implemented or throws an error if it doesn't.GeocodercallbatchGeocodecatching thenot implementederror to fallback to the pool-of-promises legacy strategyTomTomGeocoderimplements_batchGeocodecalling the TomTom Batch APIs to 1. Create a job, 2. Poll the job status 3. Download the results when it's donelimitandcountryparameters forTomTom(see TODOs)Additionally
FetchAdapterto allow the return of the full response in order to check status codes, batch API it's not as simple as getting JSON results,POSTsupport toFetchAdaptar(I haven't done for the rest of the adapter, but I believe they're deprecated, I'm I right)querystringdeprecated dependency onFetchAdapterTODOs
README.mdwith new TomTom parameters @nchaulet I'd appreciate your help here. Thanks :)