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

Store a single geometry inside Wkt #72

Closed lnicola closed 2 years ago

lnicola commented 2 years ago

r? @urschrei

michaelkirk commented 2 years ago

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 commented 2 years ago

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

michaelkirk commented 2 years ago

Oh you're right! Sorry.

lnicola commented 2 years ago

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

urschrei commented 2 years ago

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

lnicola commented 2 years ago

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

bors r+

bors[bot] commented 2 years ago

:lock: Permission denied

Existing reviewers: click here to make lnicola a reviewer

michaelkirk commented 2 years ago

Added! Can you please try again?

lnicola commented 2 years ago

bors r+

bors[bot] commented 2 years ago

Build succeeded:

lnicola commented 2 years ago

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