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

Implement Openstreetmap provider #22

Merged
merged 5 commits into from Aug 20, 2019
Merged

Conversation

pjsier
Copy link
Member

@pjsier pjsier commented Aug 19, 2019

Thanks for putting this library together! I took a stab at implementing an OpenStreetMap provider here since it was listed in #1

I tried to follow the patterns from the OpenCage provider and used the builder pattern for specifying additional parameters, but I'm new to Rust so let me know if anything seems off. I'd like to use the InputBounds struct for the viewport parameter here, but wanted to check and see what the best way of restructuring that would be

@mthh
Copy link
Member

mthh commented Aug 19, 2019

I am not (yet !) contributor to georust/geocoding but I was wondering : shouldn't it be possible, for the user, to specify the endpoint address when creating a new Openstreetmap struct?
(since it is possible to install a local instance of Nominatim, as described on http://nominatim.org/release-docs/latest/admin/Installation/)

In fact it looks already feasible with your PR, but it will requires some boilerplate (compared to having a with_endpoint constructor for example).

@urschrei
Copy link
Member

If you'd like to use the InputBounds struct, you'll have to either move it into lib.rs and make it pub, then re-import into opencage, or create a separate util module or similar and import it from there. I'd go with the former for now. Also, it looks like you'll have to do some of the arithmetic yourself for the OpenStreetMap bounds, since the call requires four coordinates, but InputBounds only specifies top left and bottom right – if you want to impl an additional method on InputBounds that does the arithmetic and returns an array, go for it!

@pjsier
Copy link
Member Author

pjsier commented Aug 19, 2019

Thanks for clarifying! I implemented moving InputBounds into lib.rs here. It looks like the viewbox parameter in Nominatim takes the same input format as OpenCage, and it seems to be working now.

I'd also be interested in adding the ability to specify a custom endpoint, is a separate constructor the most straightforward way of doing that?

@urschrei
Copy link
Member

I'd also be interested in adding the ability to specify a custom endpoint, is a separate constructor the most straightforward way of doing that?

Yep I'd just add a new_with_endpoint or similar impl. Up to you whether you'd like to perform any kind of validation using e.g. rust-url to ensure it's a valid URL.

@pjsier
Copy link
Member Author

pjsier commented Aug 19, 2019

Great, thanks! Just made that update so this should be ready for review

@urschrei
Copy link
Member

Looks good to me!

@urschrei
Copy link
Member

bors r+

bors bot added a commit that referenced this pull request Aug 20, 2019
22: Implement Openstreetmap provider r=urschrei a=pjsier

Thanks for putting this library together! I took a stab at implementing an OpenStreetMap provider here since it was listed in #1 

I tried to follow the patterns from the OpenCage provider and used the builder pattern for specifying additional parameters, but I'm new to Rust so let me know if anything seems off. I'd like to use the `InputBounds` struct for the `viewport` parameter here, but wanted to check and see what the best way of restructuring that would be

Co-authored-by: pjsier <pjsier@gmail.com>
@bors
Copy link
Contributor

bors bot commented Aug 20, 2019

Build succeeded

@bors bors bot merged commit 5eb1dc3 into georust:master Aug 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants