search

georust / wkt

Rust read/write support for well-known text (WKT)
https://crates.io/crates/wkt
Apache License 2.0
51 stars 25 forks source link

Fix parsing Z, M, and ZM WKT strings #115

Closed kylebarron closed 2 months ago

kylebarron commented 2 months ago
kylebarron commented 2 months ago

@michaelkirk since the changes from https://github.com/georust/wkt/pull/110 might be fresh on your mind, do you want to look at the merge conflicts here and/or do you have suggestions for how to resolve them? Or do you want to look at the approach I took here and maybe rebase those onto the latest main?

kylebarron commented 2 months ago

Some of the tests here come from the GEOS test suite in https://github.com/libgeos/geos/blob/8e525c825caf314d61a6279a426a59ded115681e/tests/unit/io/WKTReaderTest.cpp

michaelkirk commented 2 months ago

@michaelkirk since the changes from https://github.com/georust/wkt/pull/110 might be fresh on your mind, do you want to look at the merge conflicts here and/or do you have suggestions for how to resolve them? Or do you want to look at the approach I took here and maybe rebase those onto the latest main?

Sorry, I wouldn't have merged that if I knew you were working on this. I'll push up a resolution in a moment. The changes are pretty mechanical.

michaelkirk commented 2 months ago

I'm seeing some tests failing with your branch. Can you fix those up?

failures:

---- tests::empty_items stdout ----
thread 'tests::empty_items' panicked at src/lib.rs:380:63:
called `Option::unwrap()` on a `None` value
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- tests::lowercase_point stdout ----
thread 'tests::lowercase_point' panicked at src/lib.rs:395:63:
called `Option::unwrap()` on a `None` value

---- types::multipoint::tests::empty_multipoint stdout ----
thread 'types::multipoint::tests::empty_multipoint' panicked at src/types/multipoint.rs:151:63:
called `Result::unwrap()` on an `Err` value: "Unexpected word before open paren"

failures:
    tests::empty_items
    tests::lowercase_point
kylebarron commented 2 months ago

Sorry, I wouldn't have merged that if I knew you were working on this.

No worries! It was just bad luck!

kylebarron commented 2 months ago

tests all pass locally now!

kylebarron commented 2 months ago

Do you know why CI isn't running on this branch?

michaelkirk commented 2 months ago

I do not know why CI isn't running for your branch. Maybe related to the conflicts in the current branch? (working on those now)

michaelkirk commented 2 months ago

Ok - conflicts resolved.

ariesdevil commented 2 months ago

So we now only support POINT Z, and no longer support POINTZ?

michaelkirk commented 2 months ago

That's correct. I don't ever use 3d points, but my understanding is that "POINT Z" is the correct spelling. Please let us know if you have compelling counter examples!

kylebarron commented 2 months ago

So we now only support POINT Z, and no longer support POINTZ?

I added support for POINTZ (and LINESTRINGZ etc) in the latest commit. It does make the match statement kinda verbose; @michaelkirk what do you think?

michaelkirk commented 2 months ago

Sure, since there is apparently some precedent for this spelling, I'm OK with (happy to?) support it. Thanks @kylebarron

kylebarron commented 2 months ago

I had missed it but chapter 7 of https://www.ogc.org/standard/sfa/ does define WKT for geometries