georust / geo

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

created a simpler syntax for macros for test fixtures #1061

Closed culebron closed 10 months ago

culebron commented 10 months ago

The Problem

Creating Point/LineString/Polygon objects in geo-types seems very tedious. Macros don't seem to facilitate usage:

let ls = line_string![
    (x: -21.95156, y: 64.1446),
    (x: -21.951, y: 64.14479),
    (x: -21.95044, y: 64.14527),
    (x: -21.951445, y: 64.145508),
];

Retyping or duplicating x/y for each coord, also with all its parenthesis, feels insane.

I had to write some tests and ended up (1) first making a function that accepts &[(f32, f32)] and calls LineString::from inside, then (2) writing a macros with much simpler syntax, like in Well-Known Text format:

LINESTRING (76.940862 43.259595,76.940210 43.260048,76.939658 43.260287,76.939471 43.260082,76.939224 43.259770,76.939195 43.259505))

Proposal

Here's the shorter syntax in macro that I propose with this PR. It's very close to WKT:

let ls = line_string![21.95156 64.1446, -21.951 64.14479, -21.95044 64.14527, -21.951445 64.145508];

// typical test cases:
let ls = line_string![0.0 0.0, 100.0 0.0, 100.0 100.0, 200.0 100.0];

Compare to WKT:

LINESTRING (76.940862 43.259595,76.940210 43.260048,76.939658 43.260287)

Here's a similar syntax for polygon:

// just outer ring:
let poly1 = polygon!(0.0 0.0, 10.0 0.0, 10.0 10.0, 0.0 10.0, 0.0 0.0);
// outer ring & inner rings are enclosed in square brackets:
let poly2 = polygon!(
    [0.0 0.0, 10.0 0.0, 10.0 10.0, 0.0 10.0, 0.0 0.0], // outer
    [1.0 1.0, 2.0 1.0, 2.0 2.0, 1.0 2.0, 1.0 1.0], // inner
    [6.0 6.0, 7.0 6.0, 7.0 7.0, 6.0 7.0, 6.0 6.0], // inner
);

I want to underline that this syntax is intended for test fixtures, not for "working" code, that's why it has literals, rather than expressions (the latter would not compile with a space between them). For working code, I feel like LineString::from(&[something like coords]) is enough for most cases, where you assemble linestrings from some other sources of coords.

Criticism and alternatives

Another way would be to use wkt crate and parse some coord fixtures from strings -- well, I'd prefer to have less dependencies, and this way is still tedious, like this:

let ls: LineString<f32> = wkt::from_wkt("LINESTRING(my coords literal)").unwrap();

Forseeing some criticism, I see the comma isn't so readable and it's hard to visually split coord pairs, but it's very convenient to write tests by copying text from CSV files with WKT, e.g. from QGIS.

Also, I have my poly written in one line -- reason is to keep code compact, and to see an entire test case in one screen.

Testing

Wrote doctests and added test cases in test module, all tests pass.

culebron commented 10 months ago

I wonder how does the tests module work without any use statements?

lnicola commented 10 months ago

I wonder how does the tests module work without any use statements?

Good question :sweat_smile:.

culebron commented 10 months ago

I've added a line to CHANGES.md, please check if I got the version right.

michaelkirk commented 10 months ago

Just so you know, there is some out of band chatter happening about this change in our discord @culebron. If you aren't already in the georust discord and would like to join us, you can join at https://discord.gg/Fp2aape

Personally, I'd like to have a more concise way to specify geometries for test fixtures, so thank you for taking this on.

Instead of having something "similar to WKT", what would you think about using actual WKT?

let point = wkt!(POINT(10 20)); // returns geo_types::Point
let line_string = wkt!(LINESTRING(10 20,30 40)); // returns geo_types::LineString

Thinking about your PR inspired me to try something like this in the wkt crate: https://github.com/georust/wkt/pull/112, and I'm pretty happy with how it turned out.

lnicola commented 10 months ago

Instead of having something "similar to WKT", what would you think about using actual WKT?

I don't think that will work directly because two different versions of geo-types are involved. We'd still need to call a conversion function.

michaelkirk commented 10 months ago

Sorry I should have been clearer. I'm not proposing using the macro from the wkt crate. I'm proposing creating a new similar set of macros in geo_types.

For one, I don't want geo_types to depend on wkt (I think a little copying is better than a little dependency in this case)

And maybe more importantly, the data model for WKT is not fully compatible with geo_types::Geometry - in particular POINT EMPTY is handled differently.

lnicola commented 10 months ago

For one, I don't want geo_types to depend on wkt (I think a little copying is better than a little dependency in this case)

Note that we already have a dev-dependency (in geo, not geo-types) since https://github.com/georust/geo/pull/703. The main motivation for that was improving the IDE experience, though (the large datasets were problematic for at least one popular IDE).

culebron commented 10 months ago

I'm not registered on Discord and not planning, it's too heavy.

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.

culebron commented 10 months ago

after this PR, @michaelkirk made #1063 which features everything I suggested here and a lot more, so I close this one.