Revert self intersection behavior: line-intersect#2742
Revert self intersection behavior: line-intersect#2742smallsaucepan merged 11 commits intoTurfjs:masterfrom
line-intersect#2742Conversation
…json test case, add self intersection test
smallsaucepan
left a comment
smallsaucepan
left a comment
There was a problem hiding this comment.
Hi @pm0u. Added a couple of comments. The code change itself is fine. Would you mind adding some more tests to better lock down the current ignores behaviour with other feature types?
| t.end(); | ||
| }); | ||
|
|
||
| test("turf-line-intersect - self intersection behavior", (t) => { |
There was a problem hiding this comment.
Are there advantages to running the same features through both "in code" and as in/out fixture tests?
Would you mind adding some simple tests for MultiLineString, Polygon, and MultiPolygon as well e.g.
To be honest I'm not sure what the expected behaviour of those are. Should ignoreSelfIntersections apply to the entire MultiLineString, or only take effect within each individual LineString? If you're got the time let's take this opportunity to embed whatever the behaviour is into some baseline tests.
There was a problem hiding this comment.
Probably no reason to have both - is there a preference for one or the other? Its not clear to me why some tests are based on fixtures - is this preferred for simple T/F style testing?
There was a problem hiding this comment.
Remembering my logic here - The reason was that the tests below specifically test the behavior of the ignoreSelfIntersections param. Because the in/out fixture tests all use the same setup it can't be validated in that setting.
If we think this approach makes sense I can add a note as to why this is set up this way.
There was a problem hiding this comment.
Fixtures were probaby chosen to avoid 1000s of lines of geojson in code. If that approach doesn't fit nicely though, by all means add normal test cases.
|
Added a few more test cases, let me know if there are any additional cases we would like covered
All of the above test cases produce no intersecting geometries when |
…tions-line-intersect
…m0u/turf into pm0u/self-intersections-line-intersect
|
@smallsaucepan I believe this should be in a state to be merged. Please let me know if you find any issues at this point, I believe I addressed everything in your last round of feedback |
|
Hmm, builds are successful but not being marked as completed for some reason. Will press on with merging.
|
2 participants
Purpose: Bug fix
Partially Addresses:
#2728
#2722
line-intersectignoreSelfIntersectionstotrueignoreSelfIntersectionsand explicitly setting to false, using same geometry from the geojson test case added.