Add support for Mapzen's Geocoder #531
Conversation
|
Awesome @vicchi! Im 👍 but I do not know about the Mapzen service. I've reviewed this PR from the general providers' point of view. |
| /** | ||
| * @author Gary Gale <[email protected]> | ||
| */ | ||
| class Mapzen extends AbstractHttpProvider |
There was a problem hiding this comment.
It makes sense to finalize this class.
There was a problem hiding this comment.
That's true; it probably does. However when I wrote this I used the existing provider implementations as examples of best practice and as far as I can see, none of these finalise their classes. Not that's not a good reason not to do this, but I would have thought it would make more sense to apply finalisation across the board, rather than have a single provider finalise and none of the others doing so.
Thoughts?
There was a problem hiding this comment.
I would have thought it would make more sense to apply finalisation across the board, rather than have a single provider finalise and none of the others doing so
definitely yes!
There was a problem hiding this comment.
I've just opened a new issue (#537) so we track adding finalisation to all providers. But I'm also assuming that based on @willdurand's comment above, there's nothing stopping this PR from being merged now?
There was a problem hiding this comment.
So far this is the only provider without final keyword.
|
@willdurand - can this PR be merged now? I want to start working on the testing with encrypted provider API keys/credentials, but would prefer to have Mapzen tested as part of this. |
|
thanks @vicchi! |
|
Thanks! |
|
Thank you for writing what I could not! |
|
@vicchi I tested this finally and I got one unexpected result: the On the other providers that I use (ArcGISOnline, Bing, Nominatim) the |
This PR depends on the serialisation fix in PR #530 in order for the new provider's unit test cache files to be written correctly and for the unit tests to be repeatable, based on those cached files.