georust / geo

Geospatial primitives and algorithms for Rust
https://crates.io/crates/geo
Other
1.53k stars 198 forks source link

wkt macro to create geo-types #1063

Closed michaelkirk closed 11 months ago

michaelkirk commented 1 year ago

Inspired by #1061. I don't think it necessarily replaces #1061, but it might obviate it. It's arguable we might want both (or maybe neither 😢)... how do people feel?

I've applied the new macro to a sampling of tests to give people an idea of where it's nice / less nice.

culebron commented 1 year ago

I like this, I think this is better that what I wrote. A lot more options.

urschrei commented 1 year ago

This is great!

michaelkirk commented 1 year ago

(Replying to @culebron https://github.com/georust/geo/pull/1061#issuecomment-1727680769 here since I think it's about the changes in this PR)

I like the wkt! macro syntax. The only problem seems to be the confusion with wkt crate. Maybe a different name can work better?

I thought of a name like geom! (geom!(LINESTRING (10.0 20.0,30.0 40.0)) but it will also be confusing of whether it returns the exact type or a general enum wrapper for geometry objects.

re: enum vs inner type

I initially thought it should return a Geometry enum rather than the variants, and implemented it that way, but then when I tried to actually use it in our existing test cases, I found I was almost always immediately unwrapping it to its inner type. So I became convinced it should return the inner type (Point, LineString, etc.) rather than a Geometry. You can always Geometry::from(inner) in the less common case of actually wanting the Geometry enum.

re: naming

I think it's important to have 'wkt' in the name since it's a macro that takes WKT. One goal is to allow easy copy/paste of test fixtures from other tools. Another goal is to be concise, so a short name is preferred as long as it's clear enough.

I'm not too concerned with the naming collision of geo_types::wkt! vs the wkt crate or even the proposed and similar wkt::wkt macro.

But, I'd also be OK with something like geo_wkt! { POINT(0 1) } if there's a quorum of support for it.

urschrei commented 11 months ago

I'd like to merge this. Anyone else want to chime in? Speak now (or any time before tomorrow morning)

urschrei commented 11 months ago

now what