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

Make Trie Almost Fully Typing #6

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

RF-Tar-Railt
Copy link

@RF-Tar-Railt RF-Tar-Railt commented Mar 26, 2024

resolve #4

typing performance used pyright is the best, and pycharm can also infer.

Attention: This pull request changed require python >= 3.8.
If maintainers insists on compatibility with lower python version, I can change this to create .pyi

Copy link
Owner

@mina86 mina86 left a comment

Choose a reason for hiding this comment

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

A couple of quick notes:

  • pdm.lock and project.toml files are unnecessary.
  • Black changes bunch of formatting which increases noise in the PR. Most relevant is changing single to double quotes.

self.children = _EMPTY
self.value = _EMPTY
self.children: Children[V] = _EMPTY
self.value: Union[V, Literal[_SENTINEL]] = _SENTINEL
Copy link
Owner

Choose a reason for hiding this comment

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

This change from _EMPTY to _SENTINEL should be a separate PR since it’s unrelated to types.

stack.append(iter(items(node.children)))
path.append(None)
path.append("")
Copy link
Owner

Choose a reason for hiding this comment

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

Similarly, why is this changed from None to empty string?

Copy link
Author

@RF-Tar-Railt RF-Tar-Railt Mar 26, 2024

Choose a reason for hiding this comment

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

first, the type of path assigned to List[str] as all of _Node.iterate method'call passed list(xxx) in path. Append None to path would raise type-check error.

second, in these codes

if (not shallow or node.value is _SENTINEL) and node.children:
    stack.append(iter(items(node.children)))
    path.append(None)

while True:
    try:
        step, node = next(stack[-1])
        path[-1] = step
        break
    except StopIteration:
        stack.pop()
        path.pop()
    except IndexError:
        return

you can see that if path does have a new value appended, it will eventually be replaced by step or be popped.

Copy link
Owner

Choose a reason for hiding this comment

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

Hmm, actually empty string isn’t actually correct anyway. What path is can be overridden by subclassess if they decide to overload _path_from_key. The only requirement Trie class has is that each element of the path (what is called step in the documentation) is hashable.

So path passed to the function is a List[T]. Since the method doesn’t know what T is, it treats the list as List[Optional[T]] and uses None value.

I think _Node might need to be a generic type over S and V where S is a type which depends on how _path_from_key is defined. I’m not really sure what the best approach is.

pygtrie.py Outdated Show resolved Hide resolved
@mina86
Copy link
Owner

mina86 commented Mar 28, 2024

FYI, I’m currently suffering from lack of time so properly reviewing this may take me a few days.

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

Successfully merging this pull request may close these issues.

Type stubs
2 participants