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

Adds func GeocoderLocal to pkg openstreetmap #22

Merged
merged 2 commits into from Apr 25, 2016
Merged

Adds func GeocoderLocal to pkg openstreetmap #22

merged 2 commits into from Apr 25, 2016

Conversation

jriquelme
Copy link
Contributor

GeocoderLocal constructs an OpenStreetMap geocoder using a custom installation of Nominatim.

GeocoderLocal constructs an OpenStreetMap geocoder using a custom installation of Nominatim.
@dougnukem
Copy link
Collaborator

Thanks for the contribution.

I'd prefer if the method was more reflective of it's use (not just for local testing GeocoderLocal(nominatumURL string) e.g. GeocoderWithURL(nominatumURL string)

Or we could just have the default Geocoder constructor accept the URL as a param:

// Endpoints for Production Nominatum API base URLs
const (
    ProductionURL = "https://nominatim.openstreetmap.org/"
)

// Geocoder constructs OpenStreetMap geocoder
func Geocoder(nominaturmURL string) geo.Geocoder {
    return geo.HTTPGeocoder{
        EndpointBuilder:       baseURL(nominatimURL),
        ResponseParserFactory: func() geo.ResponseParser { return &geocodeResponse{} },
    }
}

Then to use it it'd just be:

var geocoder = openstreetmap.Geocoder(openstreetmap.ProductionURL)

@jriquelme
Copy link
Contributor Author

GeocoderWithURL(nominatimURL string) seems good to me.

Regarding to the second option, I would prefer a new function, to not break existing code due to the signature change of openstreetmap.Geocoder().

@dougnukem
Copy link
Collaborator

dougnukem commented Apr 25, 2016

👍 GeocoderWithURL(nominatumURL string) works for me, might be nice to refactor the implementation of the original constructor just to avoid duplicating code.

But that can be done in a followup PR.

@dougnukem dougnukem merged commit e31eda3 into codingsince1985:master Apr 25, 2016
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

2 participants