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

Support Null geometries #866

Merged
merged 16 commits into from
Jul 27, 2017
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/turf-helpers/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
* //=feature
*/
function feature(geometry, properties) {
if (!geometry) throw new Error('No geometry passed');
if (geometry === undefined) throw new Error('No geometry passed');

return {
type: 'Feature',
Expand Down Expand Up @@ -248,6 +248,7 @@ function multiPolygon(coordinates, properties) {
*/
function geometryCollection(geometries, properties) {
if (!geometries) throw new Error('geometries is required');
Copy link
Collaborator

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.

Copy link
Member Author

@DenisCarriere DenisCarriere Jul 27, 2017

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

Copy link
Member Author

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

Copy link
Member Author

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah! 👍 sorry, PHP confusion! 😄

Copy link
Member Author

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

if (!Array.isArray(geometries)) throw new Error('geometries must be an Array');

return feature({
type: 'GeometryCollection',
Expand Down
7 changes: 7 additions & 0 deletions packages/turf-helpers/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -385,3 +385,10 @@ test('convertArea', t => {

t.end();
});

// https://github.com/Turfjs/turf/issues/853
test('null geometries', t => {
t.equal(feature(null).geometry, null);
t.equal(featureCollection([feature(null)]).features[0].geometry, null);
t.end();
})
2 changes: 1 addition & 1 deletion packages/turf-invariant/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ export function containsNumber(coordinates: any[]): boolean;
/**
* http://turfjs.org/docs/
*/
export function getGeom(geojson: GeometryCollection | GeometryObject | Feature<any>): boolean;
export function getGeom(geojson: GeometryCollection | GeometryObject | Feature<any>): GeometryObject;

/**
* http://turfjs.org/docs/
Expand Down
13 changes: 8 additions & 5 deletions packages/turf-invariant/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ function collectionOf(featureCollection, type, name) {
* Get Geometry from Feature or Geometry Object
*
* @param {Feature<any>|Geometry<any>} geojson GeoJSON Feature or Geometry Object
* @returns {Geometry<any>} GeoJSON Geometry Object
* @returns {Geometry<any>|null} GeoJSON Geometry Object
* @throws {Error} if geojson is not a Feature or Geometry Object
* @example
* var point = {
Expand All @@ -155,8 +155,9 @@ function collectionOf(featureCollection, type, name) {
* //={"type": "Point", "coordinates": [110, 40]}
*/
function getGeom(geojson) {
if (!geojson) throw new Error('<geojson> is required');
if (geojson.geometry) return geojson.geometry;
if (geojson === undefined) throw new Error('<geojson> is required');
if (geojson === null) return null;
if (geojson.geometry !== undefined) return geojson.geometry;
if (geojson.coordinates || geojson.geometries) return geojson;
throw new Error('<geojson> must be a Feature or Geometry Object');
}
Expand All @@ -165,7 +166,7 @@ function getGeom(geojson) {
* Get Geometry Type from Feature or Geometry Object
*
* @param {Feature<any>|Geometry<any>} geojson GeoJSON Feature or Geometry Object
* @returns {string} GeoJSON Geometry Type
* @returns {string|null} GeoJSON Geometry Type
* @throws {Error} if geojson is not a Feature or Geometry Object
* @example
* var point = {
Expand All @@ -180,7 +181,9 @@ function getGeom(geojson) {
* //="Point"
*/
function getGeomType(geojson) {
return getGeom(geojson).type;
var geom = getGeom(geojson);
if (geom === null) return null;
return (geom) ? geom.type : undefined;
}

module.exports = {
Expand Down
14 changes: 14 additions & 0 deletions packages/turf-invariant/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -208,3 +208,17 @@ test('invariant#getGeomType', t => {
t.throws(() => invariant.getGeomType(collection, 'featureCollection not valid'));
t.end();
});

// https://github.com/Turfjs/turf/issues/853
test('null geometries', t => {
const nullFeature = {
type: 'Feature',
properties: {},
geometry: null
}
t.equal(invariant.getGeom(null), null);
Copy link
Collaborator

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)

t.equal(invariant.getGeomType(null), null);
t.equal(invariant.getGeom(nullFeature), null);
t.equal(invariant.getGeomType(nullFeature), null);
t.end();
});
28 changes: 28 additions & 0 deletions packages/turf-invariant/types.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import {point, lineString, polygon, geometryCollection, featureCollection} from '@turf/helpers'
import * as invariant from './'

const pt = point([0, 0])
const line = lineString([[0, 0], [1, 1]])
const poly = polygon([[[0, 0], [1, 1], [2, 2], [0, 0]]])
const gc = geometryCollection([pt.geometry, line.geometry, poly.geometry])
const fc = featureCollection([pt, line, poly])

/**
* getGeomType
*/
// invariant.getGeomType(fc) // Argument of type 'FeatureCollection<any>' is not assignable to parameter of type
invariant.getGeomType(gc)
invariant.getGeomType(pt)
invariant.getGeomType(poly)
invariant.getGeomType(line)
invariant.getGeomType(pt.geometry)

/**
* getGeom
*/
// invariant.getGeom(fc) // Argument of type 'FeatureCollection<any>' is not assignable to parameter of type
invariant.getGeom(gc)
invariant.getGeom(pt)
invariant.getGeom(line)
invariant.getGeom(poly)
invariant.getGeom(pt.geometry)
96 changes: 56 additions & 40 deletions packages/turf-meta/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,64 +48,73 @@ function coordEach(geojson, callback, excludeWrapCoord) {
wrapShrink = 0,
currentIndex = 0,
isGeometryCollection,
isFeatureCollection = geojson.type === 'FeatureCollection',
isFeature = geojson.type === 'Feature',
type = (geojson) ? geojson.type : null,
Copy link
Collaborator

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.

Copy link
Member Author

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.

isFeatureCollection = type === 'FeatureCollection',
isFeature = type === 'Feature',
stop = isFeatureCollection ? geojson.features.length : 1;

// This logic may look a little weird. The reason why it is that way
// is because it's trying to be fast. GeoJSON supports multiple kinds
// of objects at its root: FeatureCollection, Features, Geometries.
// This function has the responsibility of handling all of them, and that
// means that some of the `for` loops you see below actually just don't apply
// to certain inputs. For instance, if you give this just a
// Point geometry, then both loops are short-circuited and all we do
// is gradually rename the input until it's called 'geometry'.
//
// This also aims to allocate as few resources as possible: just a
// few numbers and booleans, rather than any temporary arrays as would
// be required with the normalization approach.
// This logic may look a little weird. The reason why it is that way
// is because it's trying to be fast. GeoJSON supports multiple kinds
// of objects at its root: FeatureCollection, Features, Geometries.
// This function has the responsibility of handling all of them, and that
// means that some of the `for` loops you see below actually just don't apply
// to certain inputs. For instance, if you give this just a
// Point geometry, then both loops are short-circuited and all we do
// is gradually rename the input until it's called 'geometry'.
//
// This also aims to allocate as few resources as possible: just a
// few numbers and booleans, rather than any temporary arrays as would
// be required with the normalization approach.
for (i = 0; i < stop; i++) {

geometryMaybeCollection = (isFeatureCollection ? geojson.features[i].geometry :
(isFeature ? geojson.geometry : geojson));
isGeometryCollection = geometryMaybeCollection.type === 'GeometryCollection';
isGeometryCollection = (geometryMaybeCollection) ? geometryMaybeCollection.type === 'GeometryCollection' : false;
stopG = isGeometryCollection ? geometryMaybeCollection.geometries.length : 1;

for (g = 0; g < stopG; g++) {
geometry = isGeometryCollection ?
geometryMaybeCollection.geometries[g] : geometryMaybeCollection;
coords = geometry.coordinates;
coords = (geometry === null) ? null : geometry.coordinates;
var geomType = (geometry === null) ? null : geometry.type;

wrapShrink = (excludeWrapCoord &&
(geometry.type === 'Polygon' || geometry.type === 'MultiPolygon')) ?
1 : 0;
wrapShrink = (excludeWrapCoord && (geomType === 'Polygon' || geomType === 'MultiPolygon')) ? 1 : 0;

if (geometry.type === 'Point') {
switch (geomType) {
case null:
Copy link
Collaborator

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

Copy link
Member Author

@DenisCarriere DenisCarriere Jul 26, 2017

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.

return;
case 'Point':
callback(coords, currentIndex);
currentIndex++;
} else if (geometry.type === 'LineString' || geometry.type === 'MultiPoint') {
break;
case 'LineString':
case 'MultiPoint':
for (j = 0; j < coords.length; j++) {
callback(coords[j], currentIndex);
currentIndex++;
}
} else if (geometry.type === 'Polygon' || geometry.type === 'MultiLineString') {
break;
case 'Polygon':
case 'MultiLineString':
for (j = 0; j < coords.length; j++)
for (k = 0; k < coords[j].length - wrapShrink; k++) {
callback(coords[j][k], currentIndex);
currentIndex++;
}
} else if (geometry.type === 'MultiPolygon') {
break;
case 'MultiPolygon':
for (j = 0; j < coords.length; j++)
for (k = 0; k < coords[j].length; k++)
for (l = 0; l < coords[j][k].length - wrapShrink; l++) {
callback(coords[j][k][l], currentIndex);
currentIndex++;
}
} else if (geometry.type === 'GeometryCollection') {
break;
case 'GeometryCollection':
for (j = 0; j < geometry.geometries.length; j++)
coordEach(geometry.geometries[j], callback, excludeWrapCoord);
} else {
throw new Error('Unknown Geometry Type');
break;
default: throw new Error('Unknown Geometry Type');
}
}
}
Expand Down Expand Up @@ -544,28 +553,33 @@ function geomEach(geojson, callback) {
(isFeature ? geojson.geometry : geojson));
geometryProperties = (isFeatureCollection ? geojson.features[i].properties :
(isFeature ? geojson.properties : {}));
isGeometryCollection = geometryMaybeCollection.type === 'GeometryCollection';
isGeometryCollection = (geometryMaybeCollection) ? geometryMaybeCollection.type === 'GeometryCollection' : false;
stopG = isGeometryCollection ? geometryMaybeCollection.geometries.length : 1;

for (g = 0; g < stopG; g++) {
geometry = isGeometryCollection ?
geometryMaybeCollection.geometries[g] : geometryMaybeCollection;

if (geometry.type === 'Point' ||
geometry.type === 'LineString' ||
geometry.type === 'MultiPoint' ||
geometry.type === 'Polygon' ||
geometry.type === 'MultiLineString' ||
geometry.type === 'MultiPolygon') {
var type = (geometry === null) ? null : geometry.type;
switch (type) {
case null:
case 'Point':
case 'LineString':
case 'MultiPoint':
case 'Polygon':
case 'MultiLineString':
case 'MultiPolygon': {
callback(geometry, currentIndex, geometryProperties);
currentIndex++;
} else if (geometry.type === 'GeometryCollection') {
break;
}
case 'GeometryCollection': {
for (j = 0; j < geometry.geometries.length; j++) {
callback(geometry.geometries[j], currentIndex, geometryProperties);
currentIndex++;
}
} else {
throw new Error('Unknown Geometry Type');
break;
}
default: throw new Error('Unknown Geometry Type');
}
}
}
Expand Down Expand Up @@ -693,9 +707,11 @@ function geomReduce(geojson, callback, initialValue) {
*/
function flattenEach(geojson, callback) {
geomEach(geojson, function (geometry, index, properties) {
var type = (geometry === null) ? null : geometry.type;

// Callback for single geometry
switch (geometry.type) {
switch (type) {
case null:
case 'Point':
case 'LineString':
case 'Polygon':
Expand All @@ -706,7 +722,7 @@ function flattenEach(geojson, callback) {
var geomType;

// Callback for multi-geometry
switch (geometry.type) {
switch (type) {
case 'MultiPoint':
geomType = 'Point';
break;
Expand Down Expand Up @@ -813,7 +829,7 @@ function flattenReduce(geojson, callback, initialValue) {
* @returns {Feature} GeoJSON Feature
*/
function feature(geometry, properties) {
if (!geometry) throw new Error('No geometry passed');
if (geometry === undefined) throw new Error('No geometry passed');

return {
type: 'Feature',
Expand Down
32 changes: 31 additions & 1 deletion packages/turf-meta/test.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,16 @@
const test = require('tape');
const {lineString} = require('@turf/helpers');
const {lineString, feature, featureCollection} = require('@turf/helpers');
const meta = require('./');
const {
featureEach,
featureReduce,
flattenEach,
flattenReduce,
geomEach,
geomReduce,
coordEach,
coordReduce
} = require('./');

const pointGeometry = {
type: 'Point',
Expand Down Expand Up @@ -408,3 +418,23 @@ test('flattenReduce#previous-feature+initialValue', t => {
t.deepEqual(sum, features[features.length - 1]);
t.end();
});

// https://github.com/Turfjs/turf/issues/853
test('null geometries', t => {
const fc = featureCollection([
feature(null),
feature(null)
]);
// Each operations
featureEach(fc, feature => t.equal(feature.geometry, null, 'featureEach'));
geomEach(fc, geometry => t.equal(geometry, null, 'geomEach'));
flattenEach(fc, feature => t.equal(feature.geometry, null, 'flattenEach'));
coordEach(fc, () => {}, 'coordEach');

// Reduce operations
t.equal(featureReduce(fc, prev => prev += 1, 0), 2, 'featureReduce');
t.equal(geomReduce(fc, prev => prev += 1, 0), 2, 'geomReduce');
t.equal(flattenReduce(fc, prev => prev += 1, 0), 2, 'flattenReduce');
t.equal(coordReduce(fc, prev => prev += 1, 0), 0, 'coordReduce');
t.end();
});