Added {From,To}Iterator for appropriate types. Also some doccomments #131
Conversation
minor refactor of existing From<X> impls
| @@ -15,6 +17,12 @@ pub struct Coordinate<T> | |||
| pub y: T, | |||
| } | |||
|
|
|||
| impl<T: Float> From<(T, T)> for Coordinate<T> { | |||
There was a problem hiding this comment.
I'm very hesitant about adding this implementation. Projects are not consistent about (x, y) vs. (y, x): https://macwright.org/lonlat/
There was a problem hiding this comment.
I was just trying to mimic the Point::new constructor, which is (x, y). I'm not 100% wedded to any one way.
Perhaps I am unsure about a fundamental point of this crate. I thought it represented geometric objects in any coordinate reference system, and hence the X/Y order makes sense, since another CRS/SRS would not be lat/lon. However if everything is aimed to be lat/lon, that changes things.
I am willing to split this part off (perhaps into another PR).
There was a problem hiding this comment.
Related: #32
I'm not sure where I stand on this. On the one hand, I'm personally in favour of coming down firmly on one side of the consensus, clearly stating that it's x, y, and letting libraries that want to use georust deal with that (which is what the ecosystem in general has to do now). On the other hand, I realise that's just my personal preference.
There was a problem hiding this comment.
After sleeping on this, I think you make a good point (heh) about Point::new. As long as we have that constructor looking like Point::new(3, 7) which implicitly is (x, y), it doesn't seem much worse to make a From impl for (T, T). For the time being, I think this new impl is fine and maybe in the future we should state in the module level documentation that all coordinate pairs in rust-geo will be x, y.
There was a problem hiding this comment.
Remember, rust-geo is still 0.x.y, so according to semver you can change things however you want. :)
| /// println!("Point x = {}, y = {}", point.x(), point.y()); | ||
| /// } | ||
| /// ``` | ||
| /// |
There was a problem hiding this comment.
With these PR I just wanted to mimic the standard constructors, so that it would easier to create those objects. Hence I didn't do anything about validity. (But the project would have decide what to do w.r.t. validity anyway ;) )
There was a problem hiding this comment.
Yes, true, and I don't think the lack of a check here is a problem at all. I'm just waiting for some intrepid soul to write the Polygon validity algorithm(s), since we can't do much checking without them anyway.
There was a problem hiding this comment.
All good points. From makes sense for now, but whenever we cross the bridge into validation-land, it should become TryFrom.
There was a problem hiding this comment.
w.r.t. validity, I think it's better to allow people to create invalid geometries and then have some sort of is_valid check, so that geometry creation is fast, and validity checks is opt-in. This is sorta like how PostGIS does it I think. (I wonder if you could implemented From (w/o validity check), and TryFrom which does do valitory check 🤔🤔 )
There was a problem hiding this comment.
A From operation definitionally "cannot fail", so this all depends on whether we consider the creation of invalid geometries to be a failure, in which case we'll have to split conversions between From and TryFrom.
| /// | ||
| /// Points can be created using the `new(x, y)` constructor, or from a `Coordinate` or pair of points. | ||
| /// | ||
| /// ``` |
There was a problem hiding this comment.
Examples sections should have an # Examples heading before the code blocks, though it's not a big deal such that we need to hold up this PR
There was a problem hiding this comment.
w.r.t. # Examples, most of the existing doc comments don't have it as a separate heading, so for consistency reasons I didn't put it in here. :)
|
bors r+ |
131: Added {From,To}Iterator for appropriate types. Also some doccomments r=frewsxcv
I added a few more `Into<X>` and `FromIterator` and `IntoIterator`, also some comments.
The doctest comments show how to use some of the new converts which should make construction of `geo` structs much easier and ergnomic.
e.g.:
let line: LineString<f32> = vec![(0., 0.), (10., 0.)].into();
Build succeeded |
I added a few more
Into<X>andFromIteratorandIntoIterator, also some comments.The doctest comments show how to use some of the new converts which should make construction of
geostructs much easier and ergnomic.e.g.: