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

wrap coordinates at poles #608

Merged
merged 3 commits into from
Aug 2, 2016
Merged

wrap coordinates at poles #608

merged 3 commits into from
Aug 2, 2016

Conversation

missinglink
Copy link
Member

@missinglink missinglink commented Jul 29, 2016

wrap coordinates at poles.

see tests for examples.

Fixes #570

@missinglink missinglink added this to the Elasticsearch 2 milestone Jul 29, 2016
@missinglink missinglink self-assigned this Jul 29, 2016
var clean = {};
var params = {
'point.lat': 40.7,
'point.lon': 195.1
Copy link
Member

Choose a reason for hiding this comment

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

Would it be clearer if this was 286.1 and the expected value was -73.9 like the control test?

@orangejulius
Copy link
Member

LGTM!

BUT I think it would be better if the expected value for all the tests were the same as the control, to make it more clear that the wrapping makes all the different values behave indentically. Disclaimer: I believe the acceptance-tests do not currently follow this guideline, I wrote the acceptance tests, and I mentioned that it would make sense to base the unit tests for wrapping off the acceptance tests. :) I'm happy to update the acceptance tests to reflect this change as well :P

@trescube
Copy link
Contributor

:shipit:

@dianashk
Copy link
Contributor

dianashk commented Aug 2, 2016

LGTM

@missinglink missinglink merged commit 935ac4f into master Aug 2, 2016
@trescube
Copy link
Contributor

trescube commented Aug 2, 2016

i'm typing for the heck of it!

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

4 participants