Comments (19)
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:
- wkt can be multidimensional (but maybe soon geo-types can be too! See #93)
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.
Related Issues (20)
- Relax ToWkt impls to work with CoordNum HOT 1
- Support deserializing integer geometries HOT 2
- release 0.11.0 HOT 2
- Add Support For EWKT
- MultiPoint should handle mix of EMPTY and non Empty points
- Should we support LINEARRING type? HOT 13
- Better way to serialize to wkt? HOT 5
- Publish WKT 0.9.0 HOT 4
- `POINT EMPTY` vis-à-vis `geo_types` HOT 2
- WKT Release: proposed minor release 0.9.1 HOT 1
- Wkt parser should parses geometry type as case insensitive HOT 2
- Next release (0.9.2) HOT 1
- Make geo-types a non-optional dependency HOT 3
- Add MIT license file HOT 1
- How to pass GEOMETRYCOLLECtion from sql HOT 1
- make Wkt<T> display HOT 3
- Use new TZM geo-types instead the custom WKT ones HOT 12
- Avoid intermediate `Wkt` allocation for `ToWkt::wkt_string` for geo-types
- Output `TRIANGLE` type? HOT 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from wkt.