-
Notifications
You must be signed in to change notification settings - Fork 135
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
Add type annotations for nodes #253
base: main
Are you sure you want to change the base?
Conversation
Allows checking with mypy usign `mypy anytree`
What is missing for this PR to be merged? |
I don't see any review comments talking about anything to change, so as far as I am aware nothing missing, waiting for maintainer. |
@c0fec0de could you please take a look? |
@CoolCat467 , does it need a PEP 561: Distributing and package type information
|
Yes, yes it does |
The Pyright type checker doesn't like your changes. This is how the intro tells us to import the Node class. from anytree import Node Using your branch, Pyright complains that Node is not exported from the $ poetry run pyright
/tmp/proj/proj.py
/tmp/proj/proj.py:1:21 - error: "Node" is not exported from module "anytree"
Import from ".node" instead (reportPrivateImportUsage)
1 error, 0 warnings, 0 informations Using version 2.12 from PyPI, Pyright accepts it. $ poetry run pyright
0 errors, 0 warnings, 0 informations |
I forget, but probably Node class lives in a submodule and is re-exported in main namespace, and I must have forgotten |
Should be fixed with a182903 |
I tried it with the documentation's first demo program. #pyright: strict
from anytree import Node, RenderTree
udo = Node("Udo")
marc = Node("Marc", parent=udo)
lian = Node("Lian", parent=marc)
dan = Node("Dan", parent=udo)
jet = Node("Jet", parent=dan)
jan = Node("Jan", parent=dan)
joe = Node("Joe", parent=dan)
for pre, fill, node in RenderTree(udo):
print("%s%s" % (pre, node.name)) Pyright doesn't know the the output types of RenderTree. $ poetry run pyright
/tmp/proj/proj.py
/tmp/proj/proj.py:13:5 - error: Type of "pre" is unknown (reportUnknownVariableType)
/tmp/proj/proj.py:13:10 - error: Type of "fill" is unknown (reportUnknownVariableType)
/tmp/proj/proj.py:13:16 - error: Type of "node" is unknown (reportUnknownVariableType)
/tmp/proj/proj.py:14:26 - error: Type of "name" is unknown (reportUnknownMemberType)
4 errors, 0 warnings, 0 informations Worse, the program fails because it can't find something called NodeMixin.
How are you testing your changes? I can share more info about my setup if it helps. |
That is sort of out of the scope of what I intended to to with this pull request. I've noticed with my work in other projects that maintainers never really like it when I make a massive pull request that adds type annotations for everything, because it's too much to review and their time is already limited, so it just sits there forever and nothing happens. I'm trying to avoid that this time by adding type annotations in steps. This particular pull request is adding type annotations for Adding type annotations for |
I get what you are saying about a small and digestible scope. We can ignore the type errors from RenderTree for now. But we can't ignore the fact that just importing it causes a NameError. from anytree import RenderTree $ poetry run python import.py
Traceback (most recent call last):
File "/tmp/proj/import.py", line 1, in <module>
from anytree import RenderTree
File "/home/isme/.cache/pypoetry/virtualenvs/proj-Kcy8vOlE-py3.12/lib/python3.12/site-packages/anytree/__init__.py", line 11, in <module>
from . import cachedsearch as cachedsearch # noqa
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/isme/.cache/pypoetry/virtualenvs/proj-Kcy8vOlE-py3.12/lib/python3.12/site-packages/anytree/cachedsearch.py", line 7, in <module>
from . import search
File "/home/isme/.cache/pypoetry/virtualenvs/proj-Kcy8vOlE-py3.12/lib/python3.12/site-packages/anytree/search.py", line 8, in <module>
from anytree.iterators import PreOrderIter
File "/home/isme/.cache/pypoetry/virtualenvs/proj-Kcy8vOlE-py3.12/lib/python3.12/site-packages/anytree/iterators/__init__.py", line 12, in <module>
from .abstractiter import AbstractIter as AbstractIter # noqa
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/isme/.cache/pypoetry/virtualenvs/proj-Kcy8vOlE-py3.12/lib/python3.12/site-packages/anytree/iterators/abstractiter.py", line 16, in <module>
NodeT = TypeVar("NodeT", bound=NodeMixin[Any] | LightNodeMixin[Any], covariant=True)
^^^^^^^^^
NameError: name 'NodeMixin' is not defined That's why I'm asking: how are you testing your changes? Can you reproduce the error? To discover it all I've done is run the first example from the docs. The owner will surely not merge broken code. One way or another that NameError needs to be fixed. I'm as enthusiastic about a typed version of this library as you are, so if you can get it working, I would consider using your fork in my apps to get at least partial type support. |
Ah, that's what you mean. Thanks for pointing that out, fixed with b313981 |
When I run the unit tests on your fork they all fail with an ImportError.
Can you reproduce this? The pyproject.toml file claims that anytree is supposed to be compatible with Python versions as old as 3.7. I think we can ignore Python 3.7 because its support life ended on 2023-06. But Python 3.8 has support life until 2024-10. I haven't tried it on Python 3.9 or above yet. |
This is because |
Last message before vacation. @CoolCat467, you may want to consider publishing the type information as a separate type stubs library. In this way you won't depend on the library author responding to your PR. We could be waiting a while :-) This is how it works for AWS's Python SDK. The SDK is in a package called boto3 and the type information is published separately in another package called boto3-stubs. You maybe be able to contibute your type information to the Typeshed project to automate most of the process for you and have the best integration exprience with type checkers. I haven't contributed to Tyoeshed before, but it's where I would start. Hope that helps! |
This pull request adds type annotations to all node types and the abstract iterator.