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

feat: LineString and MultiLineString types #17

Merged
merged 4 commits into from
Dec 11, 2024
Merged

Conversation

EvanHahn
Copy link
Contributor

@EvanHahn EvanHahn commented Dec 3, 2024

This adds support for LineString and MultiLineString geometry object types.

This adds support for [LineString] and [MultiLineString] geometry
object types.

[LineString]: https://datatracker.ietf.org/doc/html/rfc7946#section-3.1.4
[MultiLineString]: https://datatracker.ietf.org/doc/html/rfc7946#section-3.1.5
@EvanHahn EvanHahn requested a review from gmaclennan December 3, 2024 23:15
Copy link

github-actions bot commented Dec 3, 2024

The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed✅ passedDec 9, 2024, 8:28 PM

Copy link
Member

@gmaclennan gmaclennan left a comment

Choose a reason for hiding this comment

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

Looks good apart from a consistency question. For better-or-worse we omit lengths for Polygons and MultiPolygons when there is only one ring / polygon (see

lengths = lengths.length === 0 ? [rawCoords.length / 2] : lengths
). This was carried over from GeoBuf. I think it's probably over-optimization (and we did remove some of these optimizations from GeoBuf when we made this implementation, but we did not remove that one). However given that is how we encode/decode Polygon and MultiPolygon, I think maybe for consistency we should do the same for MultiLine (it's not necessary for Line). Does that make sense? Open to push back!

@EvanHahn
Copy link
Contributor Author

EvanHahn commented Dec 9, 2024

I think maybe for consistency we should do the same for MultiLine (it's not necessary for Line). Does that make sense? Open to push back!

Done in e8c1dc1. Tried to match what Geobuf does in this case. I also added some additional fixtures in cab67c0 to improve my confidence that this works.

@EvanHahn EvanHahn requested a review from gmaclennan December 9, 2024 20:29
@EvanHahn EvanHahn merged commit 0681efc into main Dec 11, 2024
3 checks passed
@EvanHahn EvanHahn deleted the linestring-types branch December 11, 2024 17:00
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.

2 participants