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

Should use Mapping, Sequence rather than Mutable versions #78

Open
abarnert opened this issue Sep 16, 2018 · 2 comments
Open

Should use Mapping, Sequence rather than Mutable versions #78

abarnert opened this issue Sep 16, 2018 · 2 comments

Comments

@abarnert
Copy link

abarnert commented Sep 16, 2018

dpath (after #35 was fixed) distinguishes between mappings, sequences, and other objects using MutableMapping and MutableSequence, instead of Mapping and Sequence. This means that it can't search within immutable sequences, like tuples:

>>> dpath.util.get([1,2,3], '1')
2
>>> dpath.util.get((1,2,3), '1')
KeyError: '1'

What about functions that need to mutate the input, like set or add? I think that even there, you want to path through using Mapping and Sequence. You may need to check at the "bottom" level, where you do the update/insert, that it's actually a MutableMapping or MutableSequence, but I'm not sure even that is necessary—and if it is, you probably still want to check Mapping or Sequence first, so you correctly report that a tuple is immutable instead of reporting that it's not something you can path into.

Consider:

>>> x = [[]]
>>> dpath.util.new(x, '0/0', 'hi')
>>> x
[['hi']]
>>> x = ([], )
>>> dpath.util.new(x, '0/0', 'hi') # should succeed and give us (['hi'],)
TypeError: Unable to path into elements of type ([],) at []
>>> x = [()]
>>> dpath.util.new(x, '0/0', 'hi') # should fail because x[0] is immutable
TypeError: Unable to path into elements of type () at [0]

(Also, the text of the TypeError is a bit weird—the problem is pathing into elements of (), or of type tuple, not of type ().)


This does raise the question of whether you want to allow pathing into strings, since isinstance(str, Sequence). If you don't want that, you'd need to explicitly exclude it with a second isinstance which checks (unicode, bytearray) for 2.x, (bytes, bytearray) for 3.0-3.4, and ByteString for 3.5+. But maybe it's fine for dpath.util.get(['abc'], '0/0') return return 'a' instead of raising a TypeError about being unable to path into strings.

@akesterson akesterson added the 3.x label Dec 30, 2019
@akesterson
Copy link
Collaborator

This is a good discussion but it could alter the interface enough that we aren't comfortable putting in 1.0. The 2.x branch is internally improving on the 1.x branch and making maintenance and certain features easier, but its interface is still compliant with 1.0. So this is slated for 3.x unless there is great need for it.

Admittedly, this is an unspoken design constraint based on dpath's original (way back when) use-case; processing JSON, which does not have tuples. We did not actually consider all possible python objects for traversal, though we did try to remain conscious of its use case outside of JSON specific use cases.

@calebcase calebcase removed the 3.x label Jan 26, 2020
@akesterson
Copy link
Collaborator

Blocked by #136

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

3 participants