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

Add structured address #34

Merged

Conversation

kruftmeister
Copy link
Collaborator

@kruftmeister kruftmeister commented Feb 10, 2017

PR to solve issue #29

It's been a while, but with Christmas, work and family added to the fact I had to sign up for many of the providers, the library supports, look into the documentation of their API, etc...

To sum up the major changes I made:

  • use context.Context with a timeout instead of time.After with the benefit of cleaning up / releasing resources in case the deadline has been reached
  • moved the structures geo.Location and geo.Address to geocoder.go, where the definition of the interface is placed, since these are part of the interface's contract and also to reduce the LOC in http_geocoder.go
  • no longer using ErrNoResults, this can be now deduced if the returned values are nil, nil; in case an error has been returned by the provider (only few really do so) it's then translated in the second return value
  • moved the examples code from geocoder_test.go to examples/geocoder_example.go to not be mistaken with unit tests, because of language specific conventions

In case of further questions / concerns, I'll be glad to discuss them.

Cheers

Rename geocoder_test.go to examples/geocoder_example.go.
Move the file to its own package to avoid import cycles.
Delete ErrNoResults, since no longer used.
Move the Location and Address structures to geocoder.go, since these are part of the interface's contract.
@kruftmeister
Copy link
Collaborator Author

Just realised I still need to update the README....

@kruftmeister
Copy link
Collaborator Author

Should be alright now.

@kruftmeister
Copy link
Collaborator Author

I just realised the build fails because of the Go version. I'm using Context from the context package, which is part of the std lib since 1.7, but in the config I see Go 1.5. So here's a question: do we want to remain at Go 1.5 and I use context.Context from golang.org/x/net/context or do we want to bump the Go version in the config?

@codingsince1985
Copy link
Owner

Upgrading Go version is a good idea, given v1.8 is around the corner.

@kruftmeister
Copy link
Collaborator Author

Right, done.

@kruftmeister
Copy link
Collaborator Author

kruftmeister commented Feb 10, 2017

Cool, I see "all checks passed". Thinking of my previous PR with failing build and the comments I've seen in the travis config

Generated encrypted environment variables of Geo API keys (won't work on forks/pull-request travis-ci builds)

Could that be a "side effect" of moving the examples in a designated file, which no longer looks like a test?

}, nil
}

func parseFloat(value string) float64 {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this func and let callers use geo.parseFloat() instead?

osm/osm.go Outdated
@@ -0,0 +1,60 @@
// Package osm provides common types for OpneStreetMap used by various providers
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo 'OpneStreetMap'

Copy link
Owner

@codingsince1985 codingsince1985 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me.

Use geo.ParseFloat in mapquest/nominatim.
Fix typo in osm/osm.go
@kruftmeister
Copy link
Collaborator Author

Thanks, merging it then :)

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