turf-union, turf-difference, turf-intersect: Swap out martinez-polygon-clipping for polygon-clipping used in v7. #1916
Conversation
|
Not sure how I'm able to request reviews, so just mentioning people. :) |
There was a problem hiding this comment.
Changes look really good, I'm just hoping to get some minor clean up before we merge it. I spent a bunch of time hand-checking the geojsons and they look correct.
Can you go into packages/turf-mask/test.js and packages/turf-dissolve/test.js and delete the test skipping logic you deleted in packages/turf-union/test.js? The geojson files should be good (or at most need to be regenerated for small changes), but the tests had to be skipped to get the build to pass when I made (#1862).
Thanks!
| if (differenced.length === 0) return null; | ||
| if (differenced.length === 1) return polygon(differenced[0], properties); |
There was a problem hiding this comment.
Can we keep this Polygon return? Seems like quite a bit of the tests changed just because of that. Otherwise we should change the types and jsdoc to match the new implementation.
There was a problem hiding this comment.
In inclined to agree with @mfedderly - it's kind of annoying when things get returned as a multipolygon when they aren't
| @@ -6,8 +6,6 @@ const write = require('write-json-file'); | |||
| const combine = require('@turf/combine').default; | |||
| const union = require('./dist/js/index.js').default; | |||
|
|
|||
| const SKIP=["union4.geojson"]; | |||
There was a problem hiding this comment.
Can you leave this skipping deleted, but keep the union4.geojson tests? The test should be fine but it was skipped here because it was failing because of the martinez-polygon-clipping issues.
packages/turf-intersect/index.ts
Outdated
| throw new Error("poly1 and poly2 must be either polygons or multiPolygons"); | ||
| } | ||
|
|
||
| // @ts-ignore |
There was a problem hiding this comment.
Can you get rid of the @ts-ignore here?
Best case you can do something that casts to the [number,number] from a number[].
function foo(data: [number, number]) { }
const data: number[] = [1, 2];
foo(data as [number,number]);
Worst case you can do something like geom1.coordinates as any or geom1.coordinates as unknown as any.
packages/turf-union/index.ts
Outdated
|
|
||
| const unioned: any = martinez.union(coords1, coords2); | ||
| // @ts-ignore |
There was a problem hiding this comment.
Please try to get rid of the @ts-ignore. See the same comment in turf-intersect/index.js.
| // @ts-ignore | ||
| // Type error because Turf allows for arbitrary coordinate dimensions. | ||
| const intersection = polygonClipping.intersection(geom1.coordinates, geom2.coordinates); | ||
| if (intersection.length === 0) return null; |
There was a problem hiding this comment.
Same comment here about trying to return a Polygon if the intersection is a single polygon and not a MultiPolygon.
| if (unioned.length === 0) return null; | ||
| if (unioned.length === 1) return polygon(unioned[0], options.properties); |
There was a problem hiding this comment.
Try to still return a Polygon here as well?
There was a problem hiding this comment.
Looking really good @mbullington - if you're able to do fix up @mfedderly comments then I think it's good to go! Thanks for following up on this long standing issue.
|
@mbullington anything I can do to help this PR get merged? |
|
@ngottlieb not sure what happened with the original author, but if you want to fork this PR and make the changes suggested I'd be happy to take a look. If I went and made the changes myself it disqualifies me from being able to merge the PR so its better if someone else does it. |
|
@mfedderly I'll try to take a look today. Have not contributed to Turf code before so have to get up to speed but the referenced changes look pretty straightforward. |
|
Hi, I took a look at all the requested changes and they sound good. Unfortunately I've been swamped the past few weeks, @ngottlieb if you're interested in helping out that would be amazing! Otherwise I can try & take a look later in the week |
|
@mbullington I submitted a PR with the changes requested -- mbullington#1 |
|
apologies if this is an awkward way of submitting it, I wasn't sure how best to add the changes to this PR |
|
@ngottlieb the changes look great, once @mbullington merges that PR I'll get this PR merged. Thanks for the help everyone! |
|
Should be ready to go. Thanks everyone for helping finish my PR! Esp @ngottlieb |
npm testat the sub modules where changes have occurred.npm run lintto ensure code style at the turf module level.Swap out all instances of
martinez-polygon-clippingforpolygon-clipping, which should fix errors expressed here: #1866I regen'ed all of the tests, then independently checked each changed output file for correctness.
I'm very new to using Lerna and just used Yarn normally, please let me know if I did something incorrect for dependencies and how to properly fix.
Huge shoutout to @rowanwins who provided the basis for most of these changes in the
v7branch!