wrap coordinates at poles #608

Merged
merged 3 commits into from Aug 2, 2016

Conversation

Projects
None yet
4 participants
@missinglink
Member

missinglink commented Jul 29, 2016

wrap coordinates at poles.

see tests for examples.

Fixes #570

@missinglink missinglink added in review and removed in progress labels Jul 29, 2016

@missinglink missinglink added this to the Elasticsearch 2 milestone Jul 29, 2016

@missinglink missinglink self-assigned this Jul 29, 2016

test/unit/sanitiser/_geo_common.js
+ var clean = {};
+ var params = {
+ 'point.lat': 40.7,
+ 'point.lon': 195.1

This comment has been minimized.

@orangejulius

orangejulius Jul 29, 2016

Member

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

@orangejulius

orangejulius Jul 29, 2016

Member

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

@orangejulius

This comment has been minimized.

Show comment
Hide comment
@orangejulius

orangejulius Jul 29, 2016

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

Member

orangejulius commented Jul 29, 2016

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

This comment has been minimized.

Show comment
Hide comment
@trescube

trescube Jul 29, 2016

Contributor

:shipit:

Contributor

trescube commented Jul 29, 2016

:shipit:

missinglink added some commits Aug 2, 2016

@dianashk

This comment has been minimized.

Show comment
Hide comment
@dianashk

dianashk Aug 2, 2016

Contributor

LGTM

Contributor

dianashk commented Aug 2, 2016

LGTM

@missinglink missinglink merged commit 935ac4f into master Aug 2, 2016

3 checks passed

approvals/lgtm this commit looks good
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@missinglink missinglink removed the in review label Aug 2, 2016

@trescube

This comment has been minimized.

Show comment
Hide comment
@trescube

trescube Aug 2, 2016

Contributor

i'm typing for the heck of it!

Contributor

trescube commented Aug 2, 2016

i'm typing for the heck of it!

je-l pushed a commit to nlsfi/pelias-api that referenced this pull request Aug 31, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment