-
Notifications
You must be signed in to change notification settings - Fork 200
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
created a simpler syntax for macros for test fixtures #1061
Conversation
I wonder how does the tests module work without any |
Good question 😅. |
I've added a line to CHANGES.md, please check if I got the version right. |
Just so you know, there is some out of band chatter happening about this change in our discord @culebron. 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?
Thinking about your PR inspired me to try something like this in the wkt crate: georust/wkt#112, and I'm pretty happy with how it turned out. |
I don't think that will work directly because two different versions of |
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 |
Note that we already have a dev-dependency (in |
I'm not registered on Discord and not planning, it's too heavy. I like the I thought of a name like |
after this PR, @michaelkirk made #1063 which features everything I suggested here and a lot more, so I close this one. |
CHANGES.md
if knowledge of this change could be valuable to users.The Problem
Creating Point/LineString/Polygon objects in geo-types seems very tedious. Macros don't seem to facilitate usage:
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 callsLineString::from
inside, then (2) writing a macros with much simpler syntax, like in Well-Known Text format:Proposal
Here's the shorter syntax in macro that I propose with this PR. It's very close to WKT:
Compare to WKT:
Here's a similar syntax for polygon:
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: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.