Bump geo-traits to 0.3#69
Conversation
| Self::PolygonType<'a>, | ||
| Self::MultiPointType<'a>, | ||
| Self::MultiLineStringType<'a>, | ||
| Self::MultiPolygonType<'a>, | ||
| Self::GeometryCollectionType<'a>, | ||
| Self::RectType<'_>, | ||
| Self::TriangleType<'_>, | ||
| Self::LineType<'_>, |
There was a problem hiding this comment.
I could use a second pair of eyes here. Some of these use the inferred lifetime and some of them use <'a>. I don't know which is right, or if they're both right?
There are a few places throughout the codebase that have similar situations
There was a problem hiding this comment.
Maybe @Kontinuation has some thoughts since they made a similar PR in #62 - or alternatively, we could update #62 now that its dependency updates have been merged.
There was a problem hiding this comment.
I think #62 still relies on unreleased features in wkt that were added since 0.13 was released (we haven't released wkt 0.14 yet)
There was a problem hiding this comment.
I could use a second pair of eyes here. Some of these use the inferred lifetime and some of them use
<'a>. I don't know which is right, or if they're both right?There are a few places throughout the codebase that have similar situations
I believe that we should use '_. Geometry sub-types should be derived using the lifetime in &'_ self, which is '_. This also makes the lifetime annotations consistent with the trait impl for &LinearRing<'a>.
There was a problem hiding this comment.
Also, please be aware that in #62 I am using concrete types such as
Point<'a>instead of using GAT (Self::PointType<'_>), that's why #62 appears to use'a. But when we switch to use GAT, the situation will become very different.
Is it better to use concrete types than the GATs? Should I switch this PR to use the concrete types with the named 'a lifetime?
There was a problem hiding this comment.
I'm not sure. Rust has too many rough edges that I am not aware of. To my best knowledge, it is OK to stick to using GATs. We don't need to switch to use concrete types here.
There was a problem hiding this comment.
I tried to standardize behavior here in 1732646 (#69). What do you think?
There was a problem hiding this comment.
I think it should be fine.
michaelkirk
left a comment
michaelkirk
left a comment
There was a problem hiding this comment.
I have a question - but, unless I'm mistaken, the answer is pretty inconsequential, so LGTM!
3 participants
CHANGES.mdif knowledge of this change could be valuable to users.Bump to geo-traits 0.3.
Note that this is on top of #68.