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

encoding/wkt: fix handling of collections with empty elements #184

Merged
merged 1 commit into from
Aug 30, 2020
Merged

encoding/wkt: fix handling of collections with empty elements #184

merged 1 commit into from
Aug 30, 2020

Conversation

erikgrinaker
Copy link
Contributor

@erikgrinaker erikgrinaker commented Aug 29, 2020

WKT encoding of collections with empty elements resulted in empty collections. Consider the following geometry:

geom.NewGeometryCollection().MustPush(
    geom.NewPointEmpty(geom.XY),
    geom.NewLineString(geom.XY),
)

Which resulted in the following encoding:

GEOMETRYCOLLECTION EMPTY

While the correct encoding is:

GEOMETRYCOLLECTION (POINT EMPTY, LINESTRING EMPTY)

The same was true for all multi-types.

This also fixes decoding of the corresponding WKT. The decoder is rather brittle, but the test cases pass at least.

@otan
Copy link
Collaborator

otan commented Aug 29, 2020

Thanks for doing this! I think you want to do the same to the MULTI* types, e.g MULTIPOINT (EMPTY, EMPTY)

@erikgrinaker
Copy link
Contributor Author

Seems so, thanks for the tip - will amend the PR.

@erikgrinaker erikgrinaker marked this pull request as draft August 29, 2020 21:40
@erikgrinaker erikgrinaker changed the title encoding/wkt: fix handling of geometry collections with empty members encoding/wkt: fix handling of collections with empty elements Aug 30, 2020
@erikgrinaker erikgrinaker marked this pull request as ready for review August 30, 2020 10:41
Copy link
Collaborator

@otan otan 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! One nit.
I can merge this and bump the CRDB version afterwards!

func readCoordsDim1(l geom.Layout, wkt string) ([]geom.Coord, string, error) {
isEmpty := strings.HasSuffix(wkt, tEmpty)
if isEmpty {
func readCoordsDim1(l geom.Layout, wkt string, allowEmpty bool) ([]geom.Coord, string, error) {
Copy link
Collaborator

@otan otan Aug 30, 2020

Choose a reason for hiding this comment

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

nit: i think allowEmpty is a bit misleading, as we allow empty just below here. maybe allowEmptyCoordinatesInBraceContents or something?

Copy link
Contributor Author

@erikgrinaker erikgrinaker Aug 30, 2020

Choose a reason for hiding this comment

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

Sure, renamed it allowEmptyCoordsInBrace

@otan otan merged commit 218e778 into twpayne:master Aug 30, 2020
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