Initial commit of turf-dissolve #556

rowanwins · 2017-01-23T10:27:40Z

Use a meaningful title for the pull request. Include the name of the package modified.
Have read How To Contribute.
Run npm test at the sub modules where changes have occured.
Run npm run lint to ensure code style at the turf module level.

First commit of turf-dissolve module.

DenisCarriere · 2017-01-23T14:11:07Z

packages/turf-dissolve/index.js

+ * @name dissolve
+ * @param {FeatureCollection<Polygon>} featureCollection input feature collection to be dissolved
+ * @param {string} propertyName property name on which to dissolve features
+ * @return {FeatureCollection} a FeatureCollection containing the dissolved polygons


JSDocs @returns instead of @return

Wouldn't it return {FeatureCollection<Polygon>}?

DenisCarriere · 2017-01-23T14:14:34Z

packages/turf-dissolve/index.js

+ *
+ * @name dissolve
+ * @param {FeatureCollection<Polygon>} featureCollection input feature collection to be dissolved
+ * @param {string} propertyName property name on which to dissolve features


Dissolving by property name should be optional.

Maybe we should introduce this option as a list of strings instead? That way this gives the ability to dissolve from multiple properties. {string|Array<string>} or only handle {Array<string>}

Happy to implement number 1, can't believe that didn't cross my mind!
Number 2 might be a bit trickier so for version 1 I'd prefer to stick to a single property.

That works for me, I just know that QGIS & ArcGIS can dissolve using multiple attributes, just something to keep in mind in the future.

@rowanwins Optional JSDocs are defined with square brackets.

* @param {string} [propertyName] ...

DenisCarriere · 2017-01-23T14:22:30Z

packages/turf-dissolve/index.js

+    }
+    rtree.load(treeItems);
+
+    for (var i = 0; i < featureCollection.features.length; i++) {


This next comment is a personal preference, but you could make this for loop like the following:

for (var polygon of featureCollection.features) { ...

Which eliminates of these lines:

for (var i = 0; i < featureCollection.features.length; i++) { var polygon = featureCollection.features[i];

This does work in the browser, I've already tested this using Webpack & Browserify

I need to hold onto the index variables unfortunately so can't really do this sorry.

Another alternative is to use the array's forEach method. I find it to be expressive like the former but offers the option to pass in the index like the latter. Since you are not breaking out of the loop and only using continue shouldn't be an issue using forEach.

@rowanwins If you need to keep the index, you can use in instead of of.

for (var i in featureCollection.features) { var polygon = featureCollection.features[i];

I dunno... personal taste I guess, I just hate seeing a for loop with i++ it feels like the jQuery days.

DenisCarriere · 2017-01-23T14:24:39Z

packages/turf-dissolve/index.js

+
+        var featureChanged = false;
+
+        for (var searchIndex = 0; searchIndex < potentialMatchingFeatures.length; searchIndex++) {


Same for loop comment from earlier

for (var polygon of potentialMatchingFeatures) { ...

DenisCarriere · 2017-01-23T14:35:09Z

Also, could you add a Typescript definition index.d.ts in the root directory of the dissolve module.

Use this as an example

https://github.com/Turfjs/turf/blob/master/packages/turf-bearing/index.d.ts

The definition should be pretty simple & straightforward.

DenisCarriere · 2017-01-23T14:36:31Z

packages/turf-dissolve/package.json

+  "name": "@turf/dissolve",
+  "version": "3.7.5",
+  "description": "Dissolves polygons based on an attribute",
+  "main": "index.js",


Add types: "index.d.ts" once theindex.d.ts is created.

Use this module as an example:
https://github.com/Turfjs/turf/tree/master/packages/turf-bearing

DenisCarriere · 2017-01-23T14:53:09Z

packages/turf/index.js

@@ -16,6 +16,7 @@ var turf = {
    within: require('@turf/within'),
    concave: require('@turf/concave'),
    difference: require('@turf/difference'),
+    dissolve: require('@turf/dissolve'),


Might want to remove this until all of the TravisCI tests pass.

We will re-add this afterwards once everything is good to go.

https://travis-ci.org/Turfjs/turf/builds/194420002#L780

Also, we will need to add the module to turf/package.json

https://github.com/Turfjs/turf/blob/master/packages/turf/package.json

DenisCarriere · 2017-01-23T23:48:06Z

packages/turf-dissolve/index.js

@@ -106,8 +106,10 @@ module.exports = function (featureCollection, propertyName) {
            }
            var matchFeature = featureCollection.features[matchFeaturePosition];

-            if (matchFeature.properties[propertyName] !== polygon.properties[propertyName]) {
-                continue;
+            if (propertyName !== null) {


Should be undefined instead of null. An empty param will result as undefined`.

Question: Did this get picked up in your tests?

happy to change it to undefined. The existing tests don't pick up any errors even though one of the polys does not have the required property.

DenisCarriere · 2017-01-23T23:48:10Z

packages/turf-dissolve/index.d.ts

+/**
+ * http://turfjs.org/docs.html#dissolve
+ */
+declare function dissolve(featureCollection: Polygons, propertyName: string): Polygons;


Include ? to make it optional.

propertyName?: string

DenisCarriere · 2017-01-23T23:52:44Z

packages/turf-dissolve/test.js

@@ -1,13 +1,18 @@
 var turfDissolve = require('./index');
 var test = require('tape');
 var polys = require('./test/polys.json');
-var expectedOutput = require('./test/outputPolys.json');
+var expectedOutput = require('./test/outputPolysByProperty.json');
+var expectedOutput2 = require('./test/outputPolysWithNoProperty.json');


Instead of hardcoding your expected results, add a fs.writeSync function using an environment variable REGEN=true to output your expected results.

Here's an example of a test case using this:

https://github.com/Turfjs/turf/blob/master/packages/turf-buffer/test/test.js#L34-L38

buffered.features = buffered.features.concat(normalize(fixture).features); if (process.env.REGEN) { fs.writeFileSync( __dirname+'/fixtures/out/'+path.split('/')[path.split('/').length-1], JSON.stringify(buffered, null, 2)); } var expected = JSON.parse(fs.readFileSync( __dirname+'/fixtures/out/'+path.split('/')[path.split('/').length-1])); t.deepEqual(expected, buffered); });

I don't follow on this? What is the point? It seems much trickier than the current approach...

DenisCarriere · 2017-01-24T00:11:06Z

packages/turf/index.js

@@ -16,7 +16,7 @@ var turf = {
    within: require('@turf/within'),
    concave: require('@turf/concave'),
    difference: require('@turf/difference'),
-    dissolve: require('@turf/dissolve'),
+    // dissolve: require('@turf/dissolve'),


Damn.. still can't make the tests pass since it really needs that @turf/dissolve module to be present for @turf/turf, once the package is published once it will be ok.

https://travis-ci.org/Turfjs/turf/builds/194629215#L2380-L2383

You can uncomment this since the tests has passed all the way to the end.

DenisCarriere · 2017-01-24T00:12:24Z

@rowanwins Looks good, just have a look at those recent comments I made and we should be good to merge and publish this module.

Include REGEN for expected fixtures in tests.
Typescript definition & JSDocs propertyName defined as optional (?)
Uncomment dissolve in turf/index.js
Catch propertyName as undefined instead of null.

DenisCarriere · 2017-01-24T13:11:09Z

FYI @rowanwins That last commit you sent 3 hours ago didn't have any updates.

DenisCarriere · 2017-01-25T17:43:05Z

@rowanwins This module is Published! 👍 🎆

Fixed some minor things: 97c458e

Initial commit of turf-dissolve

45833f6

DenisCarriere requested changes Jan 23, 2017

View reviewed changes

Changes as per requests

dfc2c7b

DenisCarriere requested changes Jan 23, 2017

View reviewed changes

DenisCarriere requested changes Jan 24, 2017

View reviewed changes

rowanwins added 2 commits January 24, 2017 21:29

initial commit of turf-dissolve

31e1ab3

resolve merge conflicts

93fd257

DenisCarriere merged commit 35c87dd into Turfjs:master Jan 25, 2017

stebogit mentioned this pull request Aug 24, 2017

Fixes #915 (booleanContains incorrect when checking 2 polygons) #917

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial commit of turf-dissolve #556

Initial commit of turf-dissolve #556

rowanwins commented Jan 23, 2017

DenisCarriere Jan 23, 2017

DenisCarriere Jan 23, 2017

rowanwins Jan 23, 2017

DenisCarriere Jan 23, 2017

DenisCarriere Jan 24, 2017

DenisCarriere Jan 23, 2017

rowanwins Jan 23, 2017

dpmcmlxxvi Jan 23, 2017

DenisCarriere Jan 23, 2017

DenisCarriere Jan 23, 2017

DenisCarriere commented Jan 23, 2017 •

edited

DenisCarriere Jan 23, 2017

DenisCarriere Jan 23, 2017 •

edited

DenisCarriere Jan 23, 2017

rowanwins Jan 24, 2017

DenisCarriere Jan 23, 2017

DenisCarriere Jan 23, 2017

rowanwins Jan 24, 2017

DenisCarriere Jan 24, 2017

DenisCarriere commented Jan 24, 2017 •

edited

DenisCarriere commented Jan 24, 2017

DenisCarriere commented Jan 25, 2017


		var featureChanged = false;

		for (var searchIndex = 0; searchIndex < potentialMatchingFeatures.length; searchIndex++) {

Initial commit of turf-dissolve #556

Initial commit of turf-dissolve #556

Conversation

rowanwins commented Jan 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DenisCarriere commented Jan 23, 2017 • edited

Choose a reason for hiding this comment

DenisCarriere Jan 23, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DenisCarriere commented Jan 24, 2017 • edited

DenisCarriere commented Jan 24, 2017

DenisCarriere commented Jan 25, 2017

DenisCarriere commented Jan 23, 2017 •

edited

DenisCarriere Jan 23, 2017 •

edited

DenisCarriere commented Jan 24, 2017 •

edited