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

@turf/line-intersect Support Geometry Objects & Collection #731

Merged
merged 3 commits into from
May 11, 2017

Conversation

dpmcmlxxvi
Copy link
Collaborator

Added support for all GeoJSON types that contain a line, polygon, or their multi-geometries. Turns out updating @turf/line-segment in #727 did most of the work. This PR mostly just updates the tests.

The updates here include:

  • Fixed the optimization that handles simple 2-vertex case. That was actually breaking the API and only handled Features as inputs. Code was getting lucky that none of the previous tests were not actually checking FeatureCollections.
  • Added tests to check input Geometry, GeometryCollection, Feature, and FeatureCollection.
    • Really just wrapped existing tests and iterated over each input type.
  • Removed prepending input GeoJSON to output GeoJSON in each test. Two reasons for doing this:
    • Having this included was breaking the deepEqual tests for MultiLineString since they get flattened to LineStrings during processing.
    • Also, saw no real need for duplicating it since it is not really testing anything. Open to suggestions if this was put in there for a reason.

Please fill in this template.

  • 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 occurred.
  • Run npm run lint to ensure code style at the turf module level.

@DenisCarriere
Copy link
Member

+1 so far, I'll have a look at it further tomorrow.

- Simplify main tests (must have the least possible LOC)
- Include separated Geometry Objects tests
- Included test case for input mutation
- Update contributors
- Compact `if` statements
CC: @dpmcmlxxvi
Copy link
Member

@DenisCarriere DenisCarriere left a comment

Choose a reason for hiding this comment

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

👍 I'm glad you caught the FeatureCollection testing

@DenisCarriere
Copy link
Member

Added Red & Blue colors to the Input fixtures, that way it's easier to see the matching intersects.

image

@DenisCarriere
Copy link
Member

DenisCarriere commented May 11, 2017

Added tests to check input Geometry, GeometryCollection, Feature, and FeatureCollection.

The tests source code was a little too intense, I've included the Geometry Object, Geometry Collection & Feature Collection in it's own test suite.

Removed prepending input GeoJSON to output GeoJSON in each test. Two reasons for doing this:

Where is this happening? The deepEqual does allow the test output to be "locked in", that way if one would refactor the code or slightly changes the index.js and something changes in the outputs then it would be detected as a failing test. At the initial steps, one would need to confirm the output manually to actually see if the output is correct, then the outputs are "locked".

@DenisCarriere DenisCarriere merged commit 1049426 into master May 11, 2017
@dpmcmlxxvi
Copy link
Collaborator Author

  • OK. I'm good with keeping them simpler. I considered using your approach but thought using the more complex GeoJSON would be a more interesting test.

  • What I was referring to was the input GeoJSON (e.g., "test/in/2-vertex-segment.geojson") gets duplicated at the beginning of the output GeoJSON ("test/out/2-vertex-segment.geojson") and then the actual output of the intersect is appended. I understand locking in the output as a fixture to check for changes in behavior but I missed the need to recheck the input. No harm either way.

@DenisCarriere
Copy link
Member

then the actual output of the intersect is appended.

Not including the input GeoJSON in the Output can be pretty difficult to understand where those intersections actually occur or *not occur.

Added Input to output results
image

Input not included to results

Very difficult to see if the results are "correct"

image

@DenisCarriere DenisCarriere deleted the update-line-intersect branch May 11, 2017 15:40
@DenisCarriere
Copy link
Member

I personally use http://geojson.io/ to visually see the <test/out>.geojson. It takes 2 sec to copy-paste the geojson and see it on a map.

@dpmcmlxxvi
Copy link
Collaborator Author

Ah, understood. That's a good point.

@DenisCarriere DenisCarriere changed the title Update @turf/line-intersect to handle all GeoJSON containers. Support Geometry Objects & Collection for @turf/line-intersect May 12, 2017
@DenisCarriere DenisCarriere changed the title Support Geometry Objects & Collection for @turf/line-intersect @turf/line-intersect Support Geometry Objects & Collection May 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants