-
Notifications
You must be signed in to change notification settings - Fork 363
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
Fix Encoder[GeometryCollection] #3288
Fix Encoder[GeometryCollection] #3288
Conversation
41a178d
to
95ad474
Compare
Hi @agrian-joel thanks a lot for your first contribution! I'll review it a bit later. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general LGTM! But I left a minor question in comments.
Thanks again!
obj.geometries.map({ | ||
case geom: Point => geom.asJson | ||
case geom: LineString => geom.asJson | ||
case geom: Polygon => geom.asJson | ||
case geom: MultiPolygon => geom.asJson | ||
case geom: MultiPoint => geom.asJson | ||
case geom: MultiLineString => geom.asJson | ||
case geom: GeometryCollection => geom.asJson | ||
case geom => throw new Exception(s"Unknown Geometry type ${geom.getClass.getName}: $geom") | ||
}).asJson |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I don't think it would make a lot of sense to throw here, the all types supported are listed here.
How about the following?
obj.geometries.collect {
case geom: Point => geom.asJson
case geom: LineString => geom.asJson
case geom: Polygon => geom.asJson
case geom: MultiPolygon => geom.asJson
case geom: MultiPoint => geom.asJson
case geom: MultiLineString => geom.asJson
case geom: GeometryCollection => geom.asJson
}.asJson
95ad474
to
5d0d210
Compare
obj.getAll[GeometryCollection].map(_.asJson) | ||
).flatten.asJson | ||
"geometries" -> | ||
obj.geometries.map({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order not to throw in case of the type mismatch, it is possible to use the collect
function instead of map
here (see #3288 (comment))
…eString should serialize those geometries once. Fixes locationtech#3167 Current behavior encodes them in geometries as both `type: MultX` and `type: GeometryCollection` Signed-off-by: Joel Anna <joel@agrian.com>
5d0d210
to
5aff455
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Overview
Encoding a GeometryCollection with a Multipolygon/MultiPoint/MultiLineString should serialize those geometries once
Current behavior encodes them in geometries as both
type: MultiX
andtype: GeometryCollection
Checklist
Closes #3167