Skip to content
This repository was archived by the owner on Jan 10, 2023. It is now read-only.

Implement discard and remove function #19

Open
pymia wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Implement discard and remove function #19

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
30 changes: 26 additions & 4 deletions pygtrie.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1364,13 +1364,35 @@ def add(self, key):
self._trie[key:] = True

def discard(self, key):
raise NotImplementedError(
'Removing keys from PrefixSet is not implemented.')
try:
node, _ = self._get_node(key)
except:
pass
Copy link
Contributor

@mina86 mina86 Sep 13, 2017

Choose a reason for hiding this comment

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

Clearly, this pass should be return, no?


if node and node.children:
Copy link
Contributor

@mina86 mina86 Sep 13, 2017

Choose a reason for hiding this comment

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

I don’t understand why you have this special case. node.children = {} is enough to remove all the children, you don’t have to iterate over them.

for child in node.children:
self.discard(child)
else:
node.children = {}
node.value = _SENTINEL
Copy link
Contributor

@mina86 mina86 Sep 13, 2017

Choose a reason for hiding this comment

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

This leaves out empty nodes which don’t lead to anything. For example, if you first add foo/bar/baz you get a prefix set with three nodes:

foo → bar → baz

Only baz’s value is set. If you now remove foo/bar you end up with an empty set which nonetheless has two nodes:

foo → bar




# Raises KeyError if elem is not contained in the set.
def remove(self, key):
raise NotImplementedError(
'Removing keys from PrefixSet is not implemented.')
try:
node, _ = self._get_node(key)
except KeyError:
return 0
Copy link
Contributor

@mina86 mina86 Sep 13, 2017

Choose a reason for hiding this comment

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

Uh? Why are you catching the exception? It should be propagated.

More importantly, why are you duplicating the implementation? It’s much better to implement discard in terms of remove, i.e.:

    def remove(self, key):
        …

    def discard(self, key):
        try:
            self.remove(key)
        except KeyError:
            pass


if node and node.children:
for child in node.children:
self.discard(child)
else:
node.children = {}
node.value = _SENTINEL

def pop(self):

raise NotImplementedError(
'Removing keys from PrefixSet is not implemented.')