-
Notifications
You must be signed in to change notification settings - Fork 934
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
Support Null geometries #866
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DenisCarriere I believe we should allow/hande only the geometry: null
case; a type
member must be defined to have a valid GeoJSON object (see specs), so null
or an object with type:null
are not valid inputs.
packages/turf-invariant/test.js
Outdated
properties: {}, | ||
geometry: null | ||
} | ||
t.equal(invariant.getGeom(null), null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DenisCarriere why supporting null
values? Reading the specs it seems the GeoJSON objects is an object and has to have a type
member (which in my understanding is the only requirement to be a valid GeoJSON object)
|
||
if (geometry.type === 'Point') { | ||
switch (geomType) { | ||
case null: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DenisCarriere same here, from the specs:
- A GeoJSON object has a member with the name "type". The value of
the member MUST be one of the GeoJSON types.
I guess the geometry
attribute might be null
, but the type
must be a valid string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section is more internal and doesn't need to reflect the exact spec. Only way to iterate over a FeatureCollection (containing valid null
geometries) with coordEach
is to not throw an error Unknown Geometry Type
since we know that null
is a valid geometry.
var geomType = (geometry === null) ? null : geometry.type;
switch (geomType) {
case null:
return;
Simply breaks the loop and moves on to the next valid geometry.
Thanks for looking to this @stebogit , I wasn't too sure how to handle this for the
👍 Yep, we should only handle the |
@stebogit ❓ Question for you: When we're using |
Good question @DenisCarriere... 🤔 |
Agreed they are valid GeoJSON features, however We could return a 🤔 still thinking how we should approach this one... |
🤔 function flattenEach(geojson, callback) {
geomEach(geojson, function (geometry, index, properties) { So the options are, we callback both |
@stebogit Chose to preserve
👍 Good point. |
@stebogit Have a glance at this one last time, I'm 👍 to merge this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DenisCarriere I added a couple of comments here, hopefully helpful
@@ -248,6 +248,7 @@ function multiPolygon(coordinates, properties) { | |||
*/ | |||
function geometryCollection(geometries, properties) { | |||
if (!geometries) throw new Error('geometries is required'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DenisCarriere just being very fussy here, but from specs geometryCollections can have an empty geometries
array.
But (!geometries)
would exclude that, right?
Same goes actually for FeatureCollection, which I'm noticing has the same check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can add those in the tests, but !geometries
would not exclude empty geometries inside, it would only exclude geometries: null
or undefined
.
lets add them to the tests and see what happens from there :) lol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(!geometries)
would not throw an error if you provide an empty Array.
$ node
> ![1,2,3]
false
> ![]
false
> !undefined
true
> !null
true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added tests for this case: c1edd39
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah! 👍 sorry, PHP confusion! 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol! Everything language has their quirks
packages/turf-meta/index.js
Outdated
@@ -32,64 +32,73 @@ function coordEach(geojson, callback, excludeWrapCoord) { | |||
wrapShrink = 0, | |||
currentIndex = 0, | |||
isGeometryCollection, | |||
isFeatureCollection = geojson.type === 'FeatureCollection', | |||
isFeature = geojson.type === 'Feature', | |||
type = (geojson) ? geojson.type : null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DenisCarriere isn't geojson required here? I mean, shouldn't this actually throw an error if !geojson
?
Then type = geojson.type
any time, as geojson.type
can be null
Sorry, type
can NOT be null
, it would be an invalid GeoJSON.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well since a Geometry
can be passed to coordEach
, we need to handle null
somehow.
Maybe its' best to add the null
handling at the very top of the function.
if (geojson === null) return;
That way the rest of the code can remain unchanged.
Adds
null
geometry support.Ref: #853
Modified
@turf/invariant
getGeom
getGeomType
getCoord
getCoords
@turf/helpers
feature
featureCollection
@turf/meta
geomEach
geomReduce
featureEach
featureReduce
flattenEach
flattenReduce
coordEach
coordReduce
Implementation Discussions
Callbacks Feature/Geometry with
null
geometrygeomEach
flattenEach
featureEach
Does NOT return any Coordinate/Segment
coordEach
segmentEach
Tested
@turf/bbox
@turf/bbox-clip