Remove ToWkt? about wkt HOT 19 CLOSED

For points it is simple, but for mulilinestings it kind of becomes a burden having to build another struct just to have a WKT string

That's a good point. Keep in mind that these are only the default implementations on the trait. Your type (or geo-types) for that matter could implement to_string without allocating the intermediate struct. I've made a TODO for that here: #93

So I guess my question at this point would be, why are the Wkt structs necessary and why the ones in geo-types weren't used?

I can't speak to the history. Two reasons that currently exist:

1. wkt can be multidimensional (but maybe soon geo-types can be too! See #93)
2. POINT EMPTY has no obvious corollary in the geo_types::Geometry enum

In any case, I'm going to close this issue at this point. The trait now has some other usage and it seems like the remaining concerns expressed here are being tracked in their own issues.

(Feel free to re-open if I'm wrong)

from wkt.

It's more discoverable than into(), but such are Rust APIs. I guess it's fine to remove it.

from wkt.

@lnicola Are you currently using ToWkt anywhere? I agree Into<ToWkt> is less discoverable, but my guess is nobody is using ToWkt to begin with

from wkt.

Ah, no, I'm not using it, but as a user I'd be fine with any reasonable API.

from wkt.

New user here. As a user, I'd love to see (and am yet to find) an easy way to convert from geo-types to Wkt for the purposes of getting the WKT string representation.

If ye permit, I can make a PR in the next hours with such impls that would make life much easier.

from wkt.

It's used in a few places at least: https://github.com/search?l=&q=wkt%3A%3AToWkt+language%3ARust&type=code

Some additional uses here, but includes some false positives from identically named traits: https://github.com/search?l=&q=ToWkt+language%3ARust&type=code

Apart from arguably being more discoverable, there might be a reason to keep it as a means of including helper methods liek the to_wkt_string() proposed in #89

from wkt.

I don't think it is actually more discoverable. For instance, it is another thing to bring into scope that you have to search the source code to know of (which I had to do).

Also, it seems like adding unrelated things to a trait that was designed to convert things to Wkt.

from wkt.

I really don't think trait ToWkt is the right abstraction. Adding one more (actually useful) method that people will depend on will tie the existence of this trait to this library forever and prevent better abstractions to be developed

from wkt.

less than 50 usages is a good number, pretty low, and a good opportunity to improve the abstractions this crate has to offer

from wkt.

I'm on the fence here. I agree that there isn't much usage (looks like three or so active crates), so deprecating it (we wouldn't be removing it immediately anyway, in line with our "try not to make dependent crate maintainers miserable" policy) isn't a major concern. I like both #88 and #89's new method, fwiw.

from wkt.

prevent better abstractions to be developed

Can you expound on this? Did you have something specific in mind? Are you thinking something more like the hypothetical traits you mentioned in #88:

What if Wkt was instead a trait itself, implemented for every type in geo_types that provided a single method: wkt(&self) -> String?

If I'm understanding correctly, I think this is a great idea, and actually close to what I was going for. For the common use case, I think the exposure of the Wkt struct introduces more concepts than necessary. What "is" Wkt? I'd guess that people almost never want the intermediate Wkt struct, and rather they just want the text String (or to write that text to some impl Write).

So to be clear, I'm imagining most people to use ToWkt instead of the Wkt struct.

as proposed in #88, I think you're advocating for:

let geo_point = geo_types::point!(x: 0.0, y: 0.0);
use wkt::Wkt;
let string = Wkt::from(geo_point).to_string()

The new From impls are an improvement over what we have now.

But I think still easier for people would be something like this:

let geo_point = geo_types::point!(x: 0.0, y: 0.0);
use wkt::ToWkt;
let string = geo_point.to_wkt_string()

Is there a different way you imagine achieving this?

from wkt.

But I think still easier for people would be something like this:

let geo_point = geo_types::point!(x: 0.0, y: 0.0);
use wkt::ToWkt;
let string = geo_point.to_wkt_string()

This seems unambiguously more ergonomic to me.

from wkt.

I just think that the current design of the ToWkt trait is not optimal. That's what I mean by "better abstractions to be developed", yes, like the hypothetical trait Wkt I was talking about.

What "is" Wkt? I'd guess that people almost never want the intermediate Wkt struct, and rather they just want the text String (or to write that text to some impl Write).

I agree with you in that people (likely) never want another struct to deal with, we already have the ones in geo_types and they should be enough. That's why I think the current design of the ToWkt trait is not optimal, it implies the existence of these wkt-specific structs. And actually I think in a more appropriate design my From impls are (obviously) not necessary.

But I think still easier for people would be something like this:

Yes, it does look better, it is less code, it is cleaner. It is just the internals what bother me.

So what are you proposing

If none of this wkt-specific structs existed and it was just the one (To)Wkt trait implemented for the structs in geo_types then the code to get the WKT string would look the same (bring trait into scope, call method). We'd probably need a few more structs in geo_types for the things like the triangle and the rectangle. If someone needs a representation of their own structs as WKT we could provide traits for every different wkt string that, if implemented, provided the desired WKT representation. That would address issues like what @grivy mentions in #32 .

Of course, that's a very heavy re-design and api-incompatible change, but I think it would be a great candidate for a 1.0 release and it is not that difficult to write.

from wkt.

I just think that the current design of the ToWkt trait is not optimal.

Sure, granted!

If none of this wkt-specific structs existed and it was just the one (To)Wkt trait implemented for the structs in geo_types then the code to get the WKT string would look the same (bring trait into scope, call method). We'd probably need a few more structs in geo_types for the things like the triangle and the rectangle. If someone needs a representation of their own structs as WKT we could provide traits for every different wkt string that, if implemented, provided the desired WKT representation. That would address issues like what @grivy mentions in #32 .

Thanks for the response. To help me understand a little more concretely, could you provide any straw-man examples — What might a user of this crate write to accomplish the tasks you've outlined?

from wkt.

What I would aim to achieve is something like this for serialization:

// the only thing to bring into scope
use wkt::Wkt;
// can use not only a Geometry but any individual type
let thing = geo_types::Polygon::new(...); // or Triangle, or something else
// write to writer
thing.write_wkt(&mut writer).unwrap();
// get string
let wkt = thing.wkt();

(yes, it does look exactly as you proposed for the string part). And for parsing:

let thing: geo_types::Polygon = wkt::parse("POLYGON (...)").unwrap();

If adding missing structs to the geo_types is not acceptable the it might be a good idea to have some internal structs just for those (and an easy way to go from these to geo_types).

As for having a WKT representation for your own structs I can see something like this:

use wkt::{Wkt, TriangleWkt, PointLike};

struct MyPoint(i32, i32);

struct MyTriangle {
    p1: MyPoint,
    p2: MyPoint,
    p3: MyPoint,
}

impl PointLike for MyPoint {
    fn get_x(&self) -> String { todo!() }
    fn get_y(&self) -> String { todo!() }
    fn get_z(&self) -> Option<String> { None }
}

impl TriangleWkt for MyTriangle {
    type Point = MyPoint;

    fn get_points(&self) -> [Self::Point; 3] { todo!() }
    fn with_z(&self) -> bool { false }
}

let t = MyTriangle { ... };
let wkt = t.wkt(); // String WKT
t.write_wkt(&mut writer).unwrap(); // to IO

from wkt.

What I would aim to achieve is something like this for serialization:

Thanks for laying out an example of what you were envisioning. I feel like I'm pretty much aligned with this part.

As for having a WKT representation for your own structs I can see something like this:

I've got a couple questions about this part -

First, impl TriangleWkt for MyTriangle - is that a typo? WKT has no Triangle type. If it's not a typo, could you explain a little more what you envision here, and why it'd be better than something like impl PolygonWkt for MyTriangle? (we decided to translate geo_type's Triangle and Rect to Polygons).

Secondly - how does this compare when laid next to #92, where I've implemented ToWkt directly on the inner Geometry variants. I understand it's not exactly what you prescribed, but it seems to solve much of the same problems.

Adding one more (actually useful) method that people will depend on will tie the existence of this trait to this library forever and prevent better abstractions to be developed

Going back to what you said a while ago, I don't think this is a very pressing concern with the wkt crate at this point. I feel like we're pretty free to break API's so long as we allow a deprecation cycle. I'm more conservative about breaking changes in the geo-types crate, since so many crates rely on the api, but I think it's less of an issue in wkt. And not really an issue at all if we provide a deprecation path.

from wkt.

Oh and note that I separated ToWkt from geo-types. So you can serialize your own types to wkt text. In your example something like:

impl ToWkt for MyPoint {
    fn to_wkt(&self) -> Wkt {        
        let item = wkt::Coord { x: self.0, y: self.1, z: None, m: None }.as_item();
        Wkt { item }
    }
}

use wkt::ToWkt;
let my_point = MyPoint(1., 2.);
let string: String = my_point.wkt_string()

(On a side note... I wonder if there's any real value at this point between Wkt vs Geometry, and if it'd be possible to consolidate them into a single type. It used to be that Wkt had potentially many geometries, but we switched it to be 1:1 a little while ago.)

from wkt.

First, impl TriangleWkt for MyTriangle - is that a typo? WKT has no Triangle type. If it's not a typo, could you explain a little more what you envision here, and why it'd be better than something like impl PolygonWkt for MyTriangle? (we decided to translate geo_type's Triangle and Rect to Polygons).

for some reason I thought that Triangle was in Wkt but not in geo-types. My bad. If Wkt types are a subset of geo-types then it makes it even easier to use geo-types instead of wkt's own for deserialization.

Oh and note that I separated ToWkt from geo-types. So you can serialize your own types to wkt text. In your example something like:

For points it is simple, but for mulilinestings it kind of becomes a burden having to build another struct just to have a WKT string

from wkt.

So I guess my question at this point would be, why are the Wkt structs necessary and why the ones in geo-types weren't used?

from wkt.