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

Improve coordinate checks #96

Merged
merged 7 commits into from
Jun 2, 2017
Merged

Improve coordinate checks #96

merged 7 commits into from
Jun 2, 2017

Conversation

hayfield
Copy link
Contributor

This makes several improvements to better check whether a coordinate is valid.

  • Fixes an exception identified in location/point/pos may exist but be empty #95
  • Deems out-of-range coordinates to be invalid
  • Maintains assertion that a (0,0) coordinate is not a correct coordinate, despite being valid against the IATI Standard (it's in the ocean and so almost certainly an incorrect value)

Tests have been added.

This is permitted against the Standard, though use of this coordinate in
actual data is almost certainly incorrect - it's in the ocean
@hayfield hayfield requested a review from dalepotter May 15, 2017 14:18
@dalepotter
Copy link
Contributor

Tests failed :-(

@@ -243,13 +243,18 @@ def valid_value(value_element):


def valid_coords(x):
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we make life easier for ourselves in future by documenting this function to our better practices...

Suggest adding a docstring here

if x == 0 and y ==0:
if x == 0 and y == 0:
return False
elif x < -90 or x > 90 or y < -180 or y > 180:
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, on the documentation front: might be worth adding a one-line comment to say what this is doing...

Copy link
Contributor

@dalepotter dalepotter left a comment

Choose a reason for hiding this comment

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

See inline comments re documentation :_)

@hayfield
Copy link
Contributor Author

Documentation added.

Also changed coordinate naming to clarify that we're talking (lat, lng) rather than a generic coordinate space.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.6%) to 55.198% when pulling 89cfd83 on blank-location into 1446164 on master.

@dalepotter dalepotter merged commit 93c863f into master Jun 2, 2017
@hayfield hayfield deleted the blank-location branch June 5, 2017 08:22
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