-
Notifications
You must be signed in to change notification settings - Fork 204
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement TryFrom<String> and TryFrom<&str> for Well-known Text for all types #820
Comments
Perhaps relevant to your interests @EvanCarroll: georust/wkt#95 |
I agree that eco system discovery is a bit of a challenge. I wonder if having Wkt as the "blessed" string representation of geometries might be confusing though. There are other equally popular alternatives. Another option would be to build in some common serializations into geo (perhaps behind feature flags?, perhaps enabled by default?).
|
Also maybe related: #564 |
I would go slightly further and say that I don't think its appropriate for Wkt to be privileged in what is ostensibly intended to be a set of high-performance geometry libraries. I think having common serialisations available by default (in the case of Wkt the dependency overhead is minimal) and some optional (geojson's are heavier) is definitely worth thinking about and discussing, though. |
@urschrei why would it impact performance to implement a trait? You realize if you don't use it, it doesn't even get included in the final build artifact. IMHO whether or not Wkt should be privileged should be a function of it's popularity. It's the only format supported everywhere. |
@urschrei didn't assert that it would impact performance to implement a trait. And he can correct me if I'm wrong, but if his concern is similar to mine, it's a concern about introducing an API that implies that WKT is the way to represent a geo-type. Specifically (and I'm repeating myself), Rather than:
I'd prefer to have something like:
Do you see a problem with that @EvanCarroll? |
No, it wouldn't be my personal preference but it would work. I don't understand the push back: GeoJSON is not an alternative to WKT, it supports only subset of geographies and it's not supported everywhere. It also requires wrapping the geometry and supports things like properties (which have nothing to do with the point). I think it would be much better to have all the geographies Not to put too much pressure on it, but most systems provide this implicit cast (postgresql) # SELECT 'POINT(0 0)'::geography;
geography
----------------------------------------------------
0101000020E610000000000000000000000000000000000000
(1 row) And, MySQL where it's explicit just knows it as text. SELECT PointFromText('POINT(0 1)'); It's also the display format for GeoPandas. |
One of my reservations is that Wkt and geo-types do not map one to one. For example, how do you represent a The inverse is also true. These are a couple of reasons why I don't think WKT makes sense to be the blessed string representation of geo-types. edit: WKT actually supports Triangles (but not Rect)! (edit edit: triangle support is mixed) |
Please assume a minimum of knowledge on my part about what
Correct!
GEOS (which
I think we all agree about its popularity, but adopting it as the default textual representation doesn't follow from that. All I – and I think @michaelkirk – are proposing here is a slightly conservative approach, since the problem space hasn't been fully explored yet. It's an approach that allows us to make an additive, non-breaking change down the line if we decide that WKT is the correct blessed textual format. |
A couple of pros to adopting wkt for Display/Debug on the geoms (mostly personal opinions):
|
Upon reflection, it probably does make more sense to just use WKT as our debug format rather than inventing our own weird similar but different thing like I proposed in #564. but I'm a little loathe to add the WKT crate as an external dependency for anybody using geo-types. I'm not 100% allergic, but feeling a little cautious about adding that baggage/churn to everybody. A couple of options that come to mind:
|
Another thing to consider is that currently the WKT crate concerns itself with the commonly compatible subset of WKT. E.g. a Similarly the WKT serializes I think that's a reasonable compromise for a crate intended for interop with other spatial libs, but I don't think that would be acceptable for Debug output. |
It seems to me that most obvious choice of a default implementation of TryFrom will go Well-known text.
I would suggest accepting a syntax such that one could do
And get a respective point. This would make it a lot easier to integrate with third parties without having to learn about the ecosystem. If the type of the Well-known text doesn't match the strict type, we can just return an invalid input error. Could also implement,
And only reject if the wkt was invalid or we didn't yet support some subset of the types that can be represented in WKT.
The text was updated successfully, but these errors were encountered: