Implement Forward and Reverse traits, add initial provider #4

urschrei · 2018-04-09T17:42:23Z

This is an implementation of forward and reverse geocoding traits, and
the provision of these traits for the OpenCage geocoding service.

The traits are deliberately split, and are deliberately minimal; any
geocoding provider should be able to return an address, and accept an
address, returning one or more Points. In order to avoid possible incompatibilities, I've re-used the Coordinate and Point structs from Geo. I'm happy to import them, but I'm aware that

a) this will blow up compile times for this crate
b) it will make interaction between the two crates less flexible.

The initial implementation makes this trait available for the OpenCage
provider, and also makes its full API responses available so that
more flexible methods can be added (that isn't WIP, but they'll land
at some point in the near future)

groteworld · 2018-04-09T18:10:40Z

Thanks for this initial push, i'll be reviewing here over the next view minutes!

groteworld · 2018-04-09T18:23:05Z

A great start to this library! The only thing that I think we should really fix now before this is released is instead of creating our own structs for geospatial features (Coordinates and Points), this project should definitely utilize rust-geo for these. All my other changes are small style/maintainability things that can be changed in days to come. But I feel this issue will set the external expectation of how one should interact with this library.

Other than that, thank you for your contribution on this, it sadly was constantly in my backlog of TODO.

urschrei · 2018-04-09T23:21:18Z

OK, we've switched over to Geo for primitives. One thing that occurred to me is that we need to be careful with handling coordinate order; Geo's Point defines x, y (or lon, lat) order, and even if it didn't, it would be disastrous if reverse-coded data from two different providers were mixed if the output order were different. Thus, all documentation makes the lon, lat order requirement clear, and notes that even though individual providers' docs may show lat, lon coordinates, Geocoding always requires lon, lat input, and returns lon, lat Point data.

urschrei · 2018-04-10T12:41:01Z

OK, I'm happy with this for now, so just gonna wait on review.

The OpenCage instance now has a method to retrieve the remaining API quota – it's using an Arc<Mutex<Option<_>>> under the hood for interior mutability, so it's thread-safe. There's obviously a little overhead associated with this, but I'm not too worried about it.
The OpenCage instance calls will return early with an Error if they encounter 4xx or 5xx responses.

groteworld

Just a few comments, not all need to be fixed before merge approval, but just trying to think through usage.

groteworld · 2018-04-10T13:19:03Z

src/opencage.rs

+    /// Create a new OpenCage geocoding instance
+    pub fn new(api_key: String) -> Self {
+        let mut headers = header::Headers::new();
+        headers.set(header::UserAgent::new(UA_STRING));


So if a developer would want to change their User-Agent string they would need to make their opencage object mutable and then go through its client instance? Is there a better way about this?

The OpenCage developer guidelines ask for a unique UA string, so they can track library usage, so this was a deliberate decision. I could add a getter / setter with the same interior mutability technique as remaining so that a mut OpenCage instance isn't necessary, but I'd also have to replace the Client instance in the same way, since its settings are retained across calls. The same applies to endpoint – its replacement or alteration requires a new Client. What do you think?

Sounds fine for now, thanks for the explanation.

groteworld · 2018-04-10T13:20:12Z

src/opencage.rs

+        Opencage {
+            api_key,
+            client,
+            endpoint: "https://api.opencagedata.com/geocode/v1/json".to_string(),


Similar with endpoint here, say the user is using a opencage ci/test api

groteworld · 2018-04-10T13:22:28Z

src/opencage.rs

+        // it's OK to index into this vec, because reverse-geocoding only returns a single result
+        let address = &res.results[0];
+        let headers = resp.headers().get::<XRatelimitRemaining>().unwrap();
+        // *self.remaining.lock().unwrap(Some(**headers));


Is this an artifact from development or is this comment purposeful still

This is an implementation of forward and reverse geocoding traits, and the provision of these traits for the OpenCage geocoding service. The traits are deliberately split, and are deliberately minimal; any geocoding provider should be able to return an address, and accept an address, returning one or more points. The initial implementation makes this trait available for the OpenCage provider, and also makes its full API responses available so that more flexible methods can be added (that isn't WIP, but they'll land at some point in the near future)

This method uses a `Cell` and interior mutability to check a custom header (`X-RateLimit-Remaining`) from an OpenCage API response, updating a private field of the OpenCage instance. The value can be retrieved using the remaining_calls() method.

It's a valid API key, so something will have to be done about that at some point.

As per API guidelines

groteworld · 2018-04-10T14:09:02Z

bors r+

4: Implement Forward and Reverse traits, add initial provider r=groteworld a=urschrei This is an implementation of forward and reverse geocoding traits, and the provision of these traits for the OpenCage geocoding service. The traits are deliberately split, and are deliberately minimal; any geocoding provider should be able to return an address, and accept an address, returning one or more Points. In order to avoid possible incompatibilities, I've re-used the `Coordinate` and `Point` structs from `Geo`. I'm happy to import them, but I'm aware that a) this will blow up compile times for this crate b) it will make interaction between the two crates less flexible. The initial implementation makes this trait available for the OpenCage provider, and also makes its full API responses available so that more flexible methods can be added (that isn't WIP, but they'll land at some point in the near future) Co-authored-by: Stephan Hügel <stephan.hugel.12@ucl.ac.uk> Co-authored-by: Stephan Hügel <urschrei@gmail.com>

groteworld · 2018-04-10T14:14:53Z

Well... bors crashed. So that's fun. I will need to investigate...

bors · 2018-04-10T14:15:13Z

Build succeeded

continuous-integration/travis-ci/push

groteworld · 2018-04-10T14:15:46Z

or nevermind? Yay?

urschrei · 2018-04-10T14:17:11Z

It often fails on that initial push on Rust-Geo, only for the actual build to complete and the merge to go through, so who knows.

groteworld · 2018-04-10T14:19:40Z

alright. publishing to crates.io now

urschrei · 2018-04-10T14:20:35Z

✨thanks!

urschrei mentioned this pull request Apr 9, 2018

Add common web services. #1

Open

3 tasks

urschrei requested a review from frewsxcv April 9, 2018 17:43

urschrei removed the request for review from frewsxcv April 10, 2018 12:14

groteworld reviewed Apr 10, 2018

View reviewed changes

urschrei force-pushed the master branch from a5b707a to b711a0f Compare April 10, 2018 13:50

urschrei and others added 15 commits April 10, 2018 14:58

Use Point from Geow

24b1b24

Be very clear about coordinate order.

debdf40

Even more explicitness about coordinate order

8f58152

Internal comments for trait implementers

53f8aaa

Typo fix

da7bdb7

Add examples to Forward and Reverse traits

02536a4

Clippy pass

4c14730

Add top-level example to OpenCage

00d5ab3

Bail out early if an http error status is returned

9fa8188

Fix up OpenCage doctest

f664284

It's a valid API key, so something will have to be done about that at some point.

Switch to an Arc and Mutex for interior mutability

1e480a3

Add custom UA header to OpenCage requests

d1e2e61

As per API guidelines

Remove unneeded line

dd9debc

urschrei force-pushed the master branch from b711a0f to dd9debc Compare April 10, 2018 13:59

bors bot merged commit dd9debc into georust:master Apr 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Forward and Reverse traits, add initial provider #4

Implement Forward and Reverse traits, add initial provider #4

urschrei commented Apr 9, 2018

groteworld commented Apr 9, 2018

groteworld commented Apr 9, 2018

urschrei commented Apr 9, 2018

urschrei commented Apr 10, 2018

groteworld left a comment

groteworld Apr 10, 2018

urschrei Apr 10, 2018

groteworld Apr 10, 2018

groteworld Apr 10, 2018

groteworld Apr 10, 2018

groteworld commented Apr 10, 2018

groteworld commented Apr 10, 2018

bors bot commented Apr 10, 2018

groteworld commented Apr 10, 2018

urschrei commented Apr 10, 2018

groteworld commented Apr 10, 2018

urschrei commented Apr 10, 2018

Implement Forward and Reverse traits, add initial provider #4

Implement Forward and Reverse traits, add initial provider #4

Conversation

urschrei commented Apr 9, 2018

groteworld commented Apr 9, 2018

groteworld commented Apr 9, 2018

urschrei commented Apr 9, 2018

urschrei commented Apr 10, 2018

groteworld left a comment

Choose a reason for hiding this comment

groteworld Apr 10, 2018

Choose a reason for hiding this comment

urschrei Apr 10, 2018

Choose a reason for hiding this comment

groteworld Apr 10, 2018

Choose a reason for hiding this comment

groteworld Apr 10, 2018

Choose a reason for hiding this comment

groteworld Apr 10, 2018

Choose a reason for hiding this comment

groteworld commented Apr 10, 2018

groteworld commented Apr 10, 2018

bors bot commented Apr 10, 2018

Build succeeded

groteworld commented Apr 10, 2018

urschrei commented Apr 10, 2018

groteworld commented Apr 10, 2018

urschrei commented Apr 10, 2018