search

praekelt / go-jsbox-location

Location finder for go-jsbox
Other
2 stars 0 forks source link

Add Open Street Map support. #22

Closed hodgestar closed 9 years ago

hodgestar commented 9 years ago

This is an alternative to #17 but only includes the Open Street Map changes, not the other features from them (e.g. skipping).

hodgestar commented 9 years ago

@imsickofmaps This isn't ready for review (still needs lots of tests) but I'm just letting you know it exists in case that's useful somehow.

imsickofmaps commented 9 years ago

@hodgestar can you let me know when you're going to pick this work up again? There's multiple rounds of QA on this project in process and we'll need to slot the OSM version in there before it's signed off.

hodgestar commented 9 years ago

Ready for review.

The fixture utilities in lib/testing.js still need updating but I'd like to leave that for another PR because this one is already too big.

justinvdm commented 9 years ago

We will probably need to update any apps using the location state, since this PR seems to introduce some breaking changes (eg, the removal of store_fields). I think it might just the directions app that uses it, but I don't really know if this was used for any other apps.

justinvdm commented 9 years ago

Looks good, left some minor comments.

rudigiesler commented 9 years ago

https://github.com/praekelt/go-google-maps pins the go-jsbox-location version to 0.1.1, so as long as this is done as a new release number, it won't break.

justinvdm commented 9 years ago

@rudigiesler Ah, that's useful, thanks. We should probably update it anyways, will make things easier if we want to use some new feature in go-google-maps at some stage in the future.

hodgestar commented 9 years ago

See #23 and #24 for future work.

hodgestar commented 9 years ago

Ready for re-review, I think.

hodgestar commented 9 years ago

@justinvdm @rudigiesler I think the version pinning in go-google-maps only affects tests. In production it will get whatever version of go-google-maps we have installed there.

justinvdm commented 9 years ago

@hodgestar Ah, good point thanks, wasn't thinking there.

gsvr commented 9 years ago

We'll also need #25 and #26.

justinvdm commented 9 years ago

:+1: if @gsvr is happy

gsvr commented 9 years ago

:+1: just some minor comments

hodgestar commented 9 years ago

@gsvr I updated the documentation for bounding_box and created #27 for improving the address label. Happy for this PR to land?

gsvr commented 9 years ago

@hodgestar Yes, happy to land :+1: