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

line-segment: Simplify implementation and add GeometryCollection test fixture #727

Merged
merged 1 commit into from
May 10, 2017
Merged

line-segment: Simplify implementation and add GeometryCollection test fixture #727

merged 1 commit into from
May 10, 2017

Conversation

dpmcmlxxvi
Copy link
Collaborator

Originally planned on adding Geometry support per #726 for line-segment until I realized that @DenisCarriere beat me to the punch. So, instead

  • Simplified the implementation using flattenEach
  • Added a GeometryCollection test fixture.

Question regarding current implementation. Is it intended for the method to silently ignore input geometries that are not lines or polygons?

I can see how that would be helpful for a user with a large feature collection that only wants the lines and polygons processed without having to create a new layer. But since Turf does a lot of input validation elsewhere, I wasn't sure if this was the intended design.


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

Question regarding current implementation. Is it intended for the method to silently ignore input geometries that are not lines or polygons?

It was intended that Point & MultiPoints are simply ignored since they don't have lines, but if one would have a FeatureCollection of random stuff (Points, Lines, Polygon) and wanted to retrieve line segments from it then it would break if one point would exist in the dataset. Currently it's not harmful to have the Points included in the FeatureCollection vs. having the method throw an error would seem more annoying at times instead of helpful.

If there's a scenario that's worth throwing an error for Points in line-segment let me know and we can discuss this further.


/**
* Creates a {@link FeatureCollection} of 2-vertex {@link LineString} segments from a {@link LineString|(Multi)LineString} or {@link Polygon|(Multi)Polygon}.
*
* @name lineSegment
* @param {Geometry|FeatureCollection|Feature<LineString|MultiLineString|MultiPolygon|Polygon>} geojson GeoJSON Polygon or LineString
* @param {GeometryCollection|Geometry|FeatureCollection|Feature<LineString|MultiLineString|MultiPolygon|Polygon>} geojson GeoJSON Polygon or LineString
Copy link
Member

@DenisCarriere DenisCarriere May 10, 2017

Choose a reason for hiding this comment

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

These JSDocs are becoming so long!... 😮
I wonder if we could shorthand the Multi Geometries

To me GeometryCollection is Geometry (it was just never supported correctly in the past).

/**
 * @param {Geometry|FeatureCollection|Feature<(Multi)LineString|(Multi)Polygon>}

default:
lineSegment(geojson, results);
}
flattenEach(geojson, function (feature) {
Copy link
Member

Choose a reason for hiding this comment

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

So Clean!! ❤️

}
geomEach(flatten(geojson), function (geometry) {
Copy link
Member

@DenisCarriere DenisCarriere May 10, 2017

Choose a reason for hiding this comment

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

Love it! 🏅 Removing LOCs is the best

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