jazzband / geojson

Python bindings and utilities for GeoJSON
https://pypi.python.org/pypi/geojson/
BSD 3-Clause "New" or "Revised" License
905 stars 120 forks source link

rewrite of validation mechanism #98

Closed zoranbosnjak closed 6 years ago

zoranbosnjak commented 7 years ago

There was a conceptual problem with validation function, since it assumes "valid" for all unknown instances. This in turn generates valid status for some otherwise false imputs (false positives). This implementation instead assumes "not implemented" unless "errors" method is implemented on all subitems.

This change introduces small incompatibility with previous version.

Test cases and README file is changed to reflect the change.

I have only small set of geojson data available. I suggest to run validation tests on bigger dataset if you have it available.

zoranbosnjak commented 7 years ago

I have fixed the comments, except I did not understand last remark:

-class Polygon(Geometry):
-    pass
+def checkPolygon(coord):
+    lengths = all([len(elem) >= 4 for elem in coord])

all([len(elem) >= 4 for elem in coord])

What needs to be changed here?

frewsxcv commented 7 years ago

What needs to be changed here?

woops, meant to write:

all(len(elem) >= 4 for elem in coord])

Also, do you happen to know if your changes fix https://github.com/frewsxcv/python-geojson/issues/99 ?

zoranbosnjak commented 7 years ago

I have converted from list to generator.

Regarding the issue #99, the validation now raises attribute error. It turns out that the features attribute is a "dict" in this partucilar case, not GeoJSON instance.

Check this:

import geojson

fc = geojson.loads("""{
    "type": "FeatureCollection",
    "features": [{}]
    }
""")

print type(fc.features[0])
print fc.errors()

>>> <type 'dict'>

I am not sure, but it might be better to fix #99 in the init, so that features list is a list of Feature instances. The validation would in this case turn False on the Feature instance and also on toplevel FeatureCollection object.

codecov-io commented 7 years ago

Codecov Report

Merging #98 into master will decrease coverage by 2.81%. The diff coverage is 84.09%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #98      +/-   ##
==========================================
- Coverage   81.23%   78.41%   -2.82%     
==========================================
  Files          12       11       -1     
  Lines         325      315      -10     
  Branches       60       52       -8     
==========================================
- Hits          264      247      -17     
- Misses         51       58       +7     
  Partials       10       10
Impacted Files Coverage Δ
geojson/__init__.py 100% <ø> (ø) :arrow_up:
geojson/_version.py 100% <100%> (ø) :arrow_up:
geojson/geometry.py 96.61% <100%> (+1.48%) :arrow_up:
geojson/feature.py 82.35% <40%> (-17.65%) :arrow_down:
geojson/base.py 78.33% <60%> (-3.67%) :arrow_down:

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 112e457...10c9713. Read the comment docs.

frewsxcv commented 6 years ago

thanks!

frewsxcv commented 6 years ago

just published 2.0.0