Skip to content

Add builtin typedefs#12

Merged
rowanwins merged 1 commit intorowanwins:masterfrom
mfedderly:mf/add-types
Mar 20, 2022
Merged

Add builtin typedefs#12
rowanwins merged 1 commit intorowanwins:masterfrom
mfedderly:mf/add-types

Conversation

@mfedderly
Copy link
Copy Markdown
Contributor

No description provided.

@rowanwins rowanwins merged commit 303d38c into rowanwins:master Mar 20, 2022
@rowanwins
Copy link
Copy Markdown
Owner

Thanks @mfedderly ! Published as 1.4.0

Comment on lines +5 to +6
export default function sweeplineIntersections(
geojson: FeatureCollection<GeometryObject> | Feature<GeometryObject>,
Copy link
Copy Markdown

@twelch twelch Mar 21, 2022

Choose a reason for hiding this comment

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

@mfedderly perhaps you had reasons, but should geojson param also accept GeometryObject, not just Feature and FeatureCollection. Also it's not all GeometryObject types according to the docs?, just LineString | MultiLineString | Polygon | MultiPolygon. So the signature would be something like:

geojson: FeatureCollection<LineString | MultiLineString | Polygon | MultiPolygon> | Feature<LineString | MultiLineString | Polygon | MultiPolygon> | LineString | MultiLineString | Polygon | MultiPolygon

See docs:
"Valid inputs: Geojson Feature or Geometry including Polygon, LineString, MultiPolygon, MultiLineString, as well as FeatureCollection."

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The second argument ignoreSelfIntersection also should be optional according to the docs

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

You are right @twelch 😂 - my review clearly stank!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

good catch @twelch !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants