Skip to content
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

Proposal: Common abstraction layer for collection types (via Deref and DerefMut) #1111

Open
RobWalt opened this issue Nov 14, 2023 · 2 comments

Comments

@RobWalt
Copy link
Contributor

RobWalt commented Nov 14, 2023

I'll try to refine and update this proposal here.

Initial Idea - Not good

The PR #1109 made me question:

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)

struct GeoVec<T>(Vec<T>);

impl<T> GeoVec<T> {
  // ... impl common functionality for all geo collection types (len, iter, ...)
}

struct MultiPoint(GeoVec<Point>);

impl Deref for MultiPoint {
  type Target = 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

impl<T> Deref for GeoVec<T> {
  type Target = Vec<T>; 
  // ... rest
}

Current Proposal

A macro or trait which unifies the common behavior MultiPoint, MultiLineString, MultiPolygon, and GeometryCollection should expose.

@michaelkirk
Copy link
Member

michaelkirk commented Nov 14, 2023

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.

@RobWalt
Copy link
Contributor Author

RobWalt commented Nov 14, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants