You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Couldn't we potentially resolve the issue by implementing Deref and DerefMut for the type instead of re-exposing the methods manually?
Obviously, this isn't a good idea since we would also re-expose a lot of other methods from Vec which wouldn't necessarily be useful for the geometry collection types.
Refined Idea - also not working
Thinking a tiny bit more about it, it might be possible to do something similar still. What if we would just do the following:
(heavily simplified to capture the essence of the idea)
structGeoVec<T>(Vec<T>);impl<T>GeoVec<T>{// ... impl common functionality for all geo collection types (len, iter, ...)}structMultiPoint(GeoVec<Point>);implDerefforMultiPoint{typeTarget = GeoVec<Point>;// ... rest}
A lot of people (myself included) use multi_polygon.0.len() already in their code bases. In case of len it wouldn't break code if we included it in the interface for GeoVec but if there is something we don't include there, then we break existing code. So we would also need to implement
it might be possible to do something similar still.
Since Deref will be applied recursively, doesn't this leave us at the same place as you were trying to avoid, mentioned here:
this isn't a good idea since we would also re-expose a lot of other methods from Vec
I confess I am kind of a Deref coward. It's useful to work around the "Orphan Rule" and for translating between Rust's zoo of smart pointers, but it has somewhat far reaching consequences, so I try to avoid it.
For example, it's harder to find methods in the documentation when they are gained via Deref, and it potentially adds a lot of API surface that you're committing to.
We're talking about 4 collections, right? MultiPoint, MultiLineString, MultiPolygon, and GeometryCollection.
As a more cowardly alternative, I'd be happy to audit and implement consistent "collection" methods on these 4 types. It could be consolidated into a private macro_rule if there is concern about consistency of implementation.
Since Deref will be applied recursively, doesn't this leave us at the same place as you were trying to avoid, mentioned here
Woops, yes. I even tried out other behavior in a playground but forgot that it traverses further down the references if a method is missing 🙈
For example, it's harder to find methods in the documentation when they are gained via Deref, and it potentially adds a lot of API surface that you're committing to.
Makes total sense. I understand and now also share your concerns! Especially because my original idea isn't working as expected and probably won't work like that anyways
As a more cowardly alternative, I'd be happy to audit and implement consistent "collection" methods on these 4 types. It could be consolidated into a private macro_rule if there is concern about consistency of implementation.
That's definitely something I would be fine with. The only other approach we would probably need to evaluate then is something like a GeoCollection trait.
I'll try to refine and update this proposal here.
Initial Idea - Not good
The PR #1109 made me question:
Obviously, this isn't a good idea since we would also re-expose a lot of other methods from
Vec
which wouldn't necessarily be useful for the geometry collection types.Refined Idea - also not working
Thinking a tiny bit more about it, it might be possible to do something similar still. What if we would just do the following:
(heavily simplified to capture the essence of the idea)
A lot of people (myself included) use
multi_polygon.0.len()
already in their code bases. In case oflen
it wouldn't break code if we included it in the interface forGeoVec
but if there is something we don't include there, then we break existing code. So we would also need to implementCurrent Proposal
A macro or trait which unifies the common behavior
MultiPoint
,MultiLineString
,MultiPolygon
, andGeometryCollection
should expose.The text was updated successfully, but these errors were encountered: