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

Fixes for issue 845 #109

Closed
wants to merge 1 commit into from
Closed

Fixes for issue 845 #109

wants to merge 1 commit into from

Conversation

rodney757
Copy link
Contributor

No description provided.

@paulproteus
Copy link
Contributor

Hi @rodney757 !

This is a good set of changes, but I'd like to ask you to revise it so it can be great! The one crucial thing is that the test suite should definitely use the code's own geocode function, rather than reimplementing it; and the second thing is that the test should basically-definitely avoid hitting the network.

If you're willing to factor it out into more commits so that the commit log can serve as historical documentation for why the lines changed, that'd be even better; but the above is the only essential one, and for simplicity I'd accept the above changes as a commit on top of this.

(But on the other hand, I love revising history with git, and I hope you gain that love too in the long run!)

@paulproteus
Copy link
Contributor

Marking "Closed" for now, pending those changes.

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