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

Errors method for validating geometry collections. #102

Merged
merged 5 commits into from Aug 30, 2017

Conversation

hrfuller
Copy link
Contributor

Closes #101

@codecov-io
Copy link

codecov-io commented Aug 24, 2017

Codecov Report

Merging #102 into master will increase coverage by 0.2%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #102     +/-   ##
=========================================
+ Coverage   78.41%   78.61%   +0.2%     
=========================================
  Files          11       11             
  Lines         315      318      +3     
  Branches       52       54      +2     
=========================================
+ Hits          247      250      +3     
  Misses         58       58             
  Partials       10       10
Impacted Files Coverage Δ
geojson/geometry.py 96.77% <100%> (+0.16%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 14f85ee...4c3dd19. Read the comment docs.

@@ -53,6 +53,9 @@ def __init__(self, geometries=None, **extra):
super(GeometryCollection, self).__init__(**extra)
self["geometries"] = geometries or []

def errors(self):
return any(list(geom.errors() for geom in self['geometries']))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think errors is supposed to return a list of errors instead of a boolean, like the other implementations

@hrfuller
Copy link
Contributor Author

hrfuller commented Aug 28, 2017

Note the any method in the version on the GeometryCollection subclass. The baseclass version wont work because an array of empty strings is truthy.

@@ -53,6 +53,13 @@ def __init__(self, geometries=None, **extra):
super(GeometryCollection, self).__init__(**extra)
self["geometries"] = geometries or []

def errors(self):
return [geom.errors() for geom in self['geometries']]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I promise this is my last comment! 🙈

I noticed that for MultiPoint, MultiLineString, and MultiPolygon, we call check_list_errors which:

  1. iterates over all the sub-geometries
  2. checks for errors for each subgeometry
  3. if there's no error, no item gets added to the resulting list

This behavior is different from your GeometryCollection behavior where the result here be something like [[], [], []].

What do you think? Would it be good to make the implementations more similar or do you think we should keep them how they are right now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem. I think it is a slightly different case than the Multi geometries because a geometry collection could contain any of those objects. However I do like the idea of consistency around errors array, so i'll change the method to only add the errors if they exist. Then we can leave the baseclass is_valid property.

@frewsxcv
Copy link
Collaborator

bors r+

bors bot added a commit that referenced this pull request Aug 30, 2017
102: Errors method for validating geometry collections. r=frewsxcv a=hrfuller

Closes #101
@frewsxcv
Copy link
Collaborator

thanks @hrfuller! 🙇

@bors
Copy link
Contributor

bors bot commented Aug 30, 2017

Build succeeded

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