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

Write custom dict-like class to use for features, geometries, and CRS #758

Closed
sgillies opened this issue Jun 10, 2019 · 8 comments
Closed
Assignees
Milestone

Comments

@sgillies
Copy link
Member

sgillies commented Jun 10, 2019

This class will raise DeprecationWarnings from its __setitem__, __setattr__, __instancecheck__, and __subclasscheck__ methods. See https://github.com/Toblerity/fiona-rfc/blob/master/rfc/0001-fiona-2-0-changes.md#considerations.

@sgillies sgillies added this to the 1.9 milestone Jun 10, 2019
@sgillies sgillies self-assigned this Jun 10, 2019
@sgillies
Copy link
Member Author

The part of this work that I was least sure about was warning when users do, for example, isinstance(dict, crs). That function call then calls dict.__instancecheck__(crs). Jake Vanderplas shows how to modify attributes of the builtin dict at https://jakevdp.github.io/blog/2017/05/26/exposing-private-dict-version/, but warns about the unknowns. I think we'll have to let instance and subclass check warnings go as things we won't fix.

@jorisvandenbossche
Copy link
Member

A dictionary also has a bunch of other methods (keys(), values(), items(), get(), update(), ...). Do you intend to keep those on the new classes (I suppose probably not all of them)? Because otherwise those might need a deprecation warning as well (although some might already give that if they use __setitem__ under the hood).

@sgillies
Copy link
Member Author

@jorisvandenbossche we get those for free when we implement MutableMapping.

@jorisvandenbossche
Copy link
Member

jorisvandenbossche commented Jun 13, 2019

Yes, you get those now from there, but my question is: do you still want those methods in the future if you make the custom Geometry or CRS object?
Because I supposed you will no longer inherit from MutableMapping then, but that's maybe a wrong assumption?

@sgillies
Copy link
Member Author

We won't implement MutableMapping, only Mapping. So, we'll get keys, values, items, get as mixin methods based on our implementation of iter and getitem. Does that answer your question?

Making features immutable is going to break a lot of my own code. But losing dict access would break even more of my own code, which is why I'd like to keep it despite the complications.

@jorisvandenbossche
Copy link
Member

Sorry, I didn't mean to imply losing key access (__getitem__) and iteration aspects of dicts, I understand that's the (logical) goal to keep that. But I mainly wanted to point out that there is more to a dict than that.

Eg if you subclass Mapping instead of MutableMapping, you loose pop, popitem, clear, update, and setdefault. So to rephrase my question: are those properly deprecated in the merged PR? (I suppose so, as they are provided as mixin methods and so are probably implemented in terms of __setitem__ or __delitem__ which are actually deprecated, but it might be good to check this).

Still inheriting from Mapping indeed gives keys, values, items, get automatically, so that part of my question is answered with that.

@sgillies
Copy link
Member Author

I've added tests to verify that we'll get a warning when calling .pop() and .update(), which in turn call __delitem__() and __setitem__().

@sgillies
Copy link
Member Author

Done!

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