Skip to content

Accept None as a key in pure python module #42

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
May 18, 2020
Merged

Accept None as a key in pure python module #42

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1a713b5
Change node indicator to an unhashable value
TIGirardi Apr 23, 2020
6aa4fa5
Merge branch 'master' into none-keys
TIGirardi May 17, 2020
3d5df27
Add tests
TIGirardi May 17, 2020
581db84
Remove unused feature from test_none_keys.py
TIGirardi May 17, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 19 additions & 12 deletions immutables/map.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,13 @@ def map_bitindex(bitmap, bit):
W_EMPTY, W_NEWNODE, W_NOT_FOUND = range(3)
void = object()

class _Unhashable:
__slots__ = ()
__hash__ = None

_NULL = _Unhashable()
Copy link
Member

@1st1 1st1 May 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd call it NULL, the module isn't imported directly anyways. Also I think you can just use void since this is just a marker that users never see.

Copy link

@jab jab May 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1. Note, void is hashable (object implements __hash__ without passing it down to subclasses), but that doesn’t matter if this is just used as a sentinel object anyway?

>>> foo = object()
>>> hash(foo)
9007199254740991

Copy link
Contributor Author

@TIGirardi TIGirardi May 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I'm being a little paranoid, but I would prefer an immediate error, which void wouldn't do. This is the current behavior:

>>> from immutables.map import Map
>>> m = Map().set(None, 1)
# do a bunch of stuff
>>> None in m
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "$prefix/lib/site-packages/immutables/map.py", line 592, in __contains__
    self.__root.find(0, map_hash(key), key)
  File "$prefix/lib/site-packages/immutables/map.py", line 157, in find
    return val_or_node.find(shift + 5, hash, key)
AttributeError: 'int' object has no attribute 'find'

With the patch (if somehow the user uses _NULL as key) it raises a TypeError immediately.

Whatever you decide, I can change with a button click.

del _Unhashable


class BitmapNode:

Expand All @@ -70,7 +77,7 @@ def assoc(self, shift, hash, key, val, mutid):
key_or_null = self.array[key_idx]
val_or_node = self.array[val_idx]

if key_or_null is None:
if key_or_null is _NULL:
sub_node, added = val_or_node.assoc(
shift + 5, hash, key, val, mutid)
if val_or_node is sub_node:
Expand Down Expand Up @@ -111,12 +118,12 @@ def assoc(self, shift, hash, key, val, mutid):
mutid)

if mutid and mutid == self.mutid:
self.array[key_idx] = None
self.array[key_idx] = _NULL
self.array[val_idx] = sub_node
return self, True
else:
ret = self.clone(mutid)
ret.array[key_idx] = None
ret.array[key_idx] = _NULL
ret.array[val_idx] = sub_node
return ret, True

Expand Down Expand Up @@ -153,7 +160,7 @@ def find(self, shift, hash, key):
key_or_null = self.array[key_idx]
val_or_node = self.array[val_idx]

if key_or_null is None:
if key_or_null is _NULL:
return val_or_node.find(shift + 5, hash, key)

if key == key_or_null:
Expand All @@ -173,7 +180,7 @@ def without(self, shift, hash, key, mutid):
key_or_null = self.array[key_idx]
val_or_node = self.array[val_idx]

if key_or_null is None:
if key_or_null is _NULL:
res, sub_node = val_or_node.without(shift + 5, hash, key, mutid)

if res is W_EMPTY:
Expand All @@ -182,7 +189,7 @@ def without(self, shift, hash, key, mutid):
elif res is W_NEWNODE:
if (type(sub_node) is BitmapNode and
sub_node.size == 2 and
sub_node.array[0] is not None):
sub_node.array[0] is not _NULL):

if mutid and mutid == self.mutid:
self.array[key_idx] = sub_node.array[0]
Expand Down Expand Up @@ -231,7 +238,7 @@ def keys(self):
for i in range(0, self.size, 2):
key_or_null = self.array[i]

if key_or_null is None:
if key_or_null is _NULL:
val_or_node = self.array[i + 1]
yield from val_or_node.keys()
else:
Expand All @@ -242,7 +249,7 @@ def values(self):
key_or_null = self.array[i]
val_or_node = self.array[i + 1]

if key_or_null is None:
if key_or_null is _NULL:
yield from val_or_node.values()
else:
yield val_or_node
Expand All @@ -252,7 +259,7 @@ def items(self):
key_or_null = self.array[i]
val_or_node = self.array[i + 1]

if key_or_null is None:
if key_or_null is _NULL:
yield from val_or_node.items()
else:
yield key_or_null, val_or_node
Expand All @@ -269,8 +276,8 @@ def dump(self, buf, level): # pragma: no cover

pad = ' ' * (level + 2)

if key_or_null is None:
buf.append(pad + 'None:')
if key_or_null is _NULL:
buf.append(pad + 'NULL:')
val_or_node.dump(buf, level + 2)
else:
buf.append(pad + '{!r}: {!r}'.format(key_or_null, val_or_node))
Expand Down Expand Up @@ -328,7 +335,7 @@ def assoc(self, shift, hash, key, val, mutid):

else:
new_node = BitmapNode(
2, map_bitpos(self.hash, shift), [None, self], mutid)
2, map_bitpos(self.hash, shift), [_NULL, self], mutid)
return new_node.assoc(shift, hash, key, val, mutid)

def without(self, shift, hash, key, mutid):
Expand Down
Loading