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

Remove Data types from the interface #158

Closed
aleokdev opened this issue Feb 12, 2022 · 1 comment · Fixed by #163
Closed

Remove Data types from the interface #158

aleokdev opened this issue Feb 12, 2022 · 1 comment · Fixed by #163
Labels
breaking This change breaks backwards-compatibility enhancement

Comments

@aleokdev
Copy link
Contributor

#135 forced us to think about some way to replace GIDs with something else that would uniquely identify a tile. References were out of the question since lifetimes would have been a pain to deal with; As such, we ended up on a design which divided types into two categories: Data and MapWrapper<'map, Data>. The first one stores all the raw data related to the object and fully owns it; The second one has a reference to the data and its parent map. This design was chosen because it would allow to have layer functions which relied in map information without having the user pass the map into these functions.

The issue now is that, although this division makes sense internally, user-side it is a mess. Functions that require the map are put in the wrapper, while everything else is in the Data member accessed through .data(). Sometimes the usage of .data() feels somewhat random; it is also inconsistent for some types such as LayerType, which has no data counterpart and is actually not a MapWrapper specialization.
As of #157 the interface looks like this:
Screenshot_20220212_160822

I suggest to make Data types internal, and replace them in the interface with properties in their respective wrappers. The main disadvantage of this approach is that it adds a lot of boilerplate code to the library (one function per data member) but it has a lot of advantages such as being able to control more precisely what the user gets access to, and having a much simpler interface.

What do you think?

@aleokdev aleokdev added enhancement breaking This change breaks backwards-compatibility labels Feb 12, 2022
@bjorn
Copy link
Member

bjorn commented Feb 14, 2022

Thanks for the nice visual overview! Your proposal makes a lot of sense to me. It'd be great if we can get rid of the .data() calls this way.

In C++ it is common practice to keep the internal data structure private and only accessible through member functions, in the interest of keeping binary compatibility. I guess this kind of compatibility is also useful in Rust, maybe even more so because adding members can even break source compatibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This change breaks backwards-compatibility enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants