Store a single geometry inside Wkt

[lnicola]

• I agree to follow the project's code of conduct.
• I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

---

r? @urschrei

[lnicola]

Wkt<T>(Geometry<T>) might be better here? Not sure.

[michaelkirk]

No strong opinion from me, but I have a slight preference for how you have it.

[lnicola]

I think I'd prefer renaming Geometry to Wkt or an into_geometry() method.

[urschrei]

No strong opinion from me, but I have a slight preference for how you have it.

Same, but do what you feel is best here. Where does the into_geometry() method come into it?

[lnicola]

Where does the into_geometry() method come into it?

I meant Wkt::into_geometry(self) -> Geometry.

I'd say keep item for now and maybe change it in a follow-up PR if we figure out a better way.

[michaelkirk]

I just noticed that cargo test-all-features doesn't seem to account for the "implicit features" of optional dependencies.

$ cargo test --all-features                                                           
   Compiling wkt v0.9.2 (/Users/mkirk/src/georust/wkt)    
warning: `#[macro_use]` only has an effect on `extern crate` and modules
  --> src/conversion.rs:50:1    
   | 
50 | #[macro_use]                  
   | ^^^^^^^^^^^^                                                                     
   |
   = note: `#[warn(unused_attributes)]` on by default      
                                           
error[E0609]: no field `items` on type `Wkt<_>`       
  --> src/deserialize.rs:75:16
   |
75 |         if wkt.items.len() == 1 {
   |                ^^^^^ help: a field with a similar name exists: `item`

error[E0308]: mismatched types
  --> src/deserialize.rs:75:31
   |
62 | impl<'de, T> Visitor<'de> for GeometryVisitor<T>
   |           - this type parameter
...
75 |         if wkt.items.len() == 1 {
   |                               ^ expected type parameter `T`, found integer
   |

[lnicola]

I think it actually failed, but I fixed it in the meantime and did a force-push.

[michaelkirk]

Oh you're right! Sorry.

[michaelkirk]

I have one non-blocking reservation: Does this break conformance with the spec? I pretty quickly surrendered before the very long and (to me) unnavigable spec.

More pragmatically, I tested that this seems to align us with the behavior of postgis: Like the highlander, There can be only one top level geometry.

eg. postgis will not parse an entity like:

sql=# SELECT ST_GeomFromText('POINT(1 2) POINT(2 3)');
ERROR:  parse error - invalid geometry
HINT:  "POINT(1 2) POINT(2 " <-- parse error at position 19 within geometry
sql=# SELECT ST_GeomFromText('POINT(1 2), POINT(2 3)');
ERROR:  parse error - invalid geometry
HINT:  "POINT(1 2), PO" <-- parse error at position 14 within geometry

So, the only way to have multiple geometries in postgis, like in this PR, is to explicitly wrap with a Geometry Collection:

SELECT ST_GeomFromText('GEOMETRYCOLLECTION(POINT(1 2), POINT(2 3))');

So this LGTM, but let's see what @urschrei thinks.

[lnicola]

It's just as conforming as the parser, since it (before my PR) parsed only one geometry out of the string.

[urschrei]

I feel the same: we're in line with PostGIS, in a less confusing way than before, so lgtm.

[lnicola]

No opposition to merging and @urschrei said it's fine to merge this before #71, so let's do that.

bors r+

[bors]

🔒 Permission denied

Existing reviewers: click here to make lnicola a reviewer

[michaelkirk]

Added! Can you please try again?

[lnicola]

bors r+

[bors]

Build succeeded:

• wkt

[lnicola]

I like how it almost takes longer to install cargo-all-features than it takes to run all the tests.