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

Add type annotations for nodes #253

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

CoolCat467
Copy link

This pull request adds type annotations to all node types and the abstract iterator.

Allows checking with mypy usign `mypy anytree`
@moisesluza
Copy link

What is missing for this PR to be merged?

@CoolCat467
Copy link
Author

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.

@moisesluza
Copy link

@c0fec0de could you please take a look?

@iainelder
Copy link

@CoolCat467 , does it need a py.typed file?

PEP 561: Distributing and package type information

Package maintainers who wish to support type checking of their code MUST add a marker file named py.typed to their package supporting typing.

@CoolCat467
Copy link
Author

Yes, yes it does

@iainelder
Copy link

iainelder commented Jul 31, 2024

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 anytree module.

$ 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 

@CoolCat467
Copy link
Author

I forget, but probably Node class lives in a submodule and is re-exported in main namespace, and I must have forgotten from <path> import Node as Node to tell typechecker it's meant to be publicly exported

@CoolCat467
Copy link
Author

Should be fixed with a182903

@iainelder
Copy link

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.

$ poetry run python proj.py
Traceback (most recent call last):
  File "/tmp/proj/proj.py", line 3, in <module>
    from anytree import Node, 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

How are you testing your changes? I can share more info about my setup if it helps.

@CoolCat467
Copy link
Author

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 Nodes and the abstract iterator since nodes use the abstract iterator internally. I worry that if I start adding more things, this pull request will remain in unmerged limbo forever.

Adding type annotations for RenderTree and friends will certainly be something to do in another pull request, but I would like to see this one merged before I spend my time on working on other pull requests in what's starting to look like a dead project.

@iainelder
Copy link

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.

@CoolCat467
Copy link
Author

Ah, that's what you mean. Thanks for pointing that out, fixed with b313981

@iainelder
Copy link

When I run the unit tests on your fork they all fail with an ImportError.

ImportError: cannot import name 'TypeGuard' from 'typing' (/usr/lib/python3.8/typing.py)

tox writes a lot of output. I've copied just the start of the unit testing output that shows the first error.

py: commands[3]> poetry run coverage run --source=anytree --branch -m pytest '--doctest-glob=docs/*.rst' --doctest-modules '--ignore-glob=tests/testdata*' --ignore=docs/conf.py --log-level=DEBUG -vv --junitxml=report.xml
================================================ test session starts =================================================
platform linux -- Python 3.8.10, pytest-6.2.5, py-1.11.0, pluggy-1.2.0 -- /tmp/fork/anytree/.tox/py/bin/python
cachedir: .tox/py/.pytest_cache
rootdir: /tmp/fork/anytree
collected 20 items / 70 errors                                                                                       

======================================================= ERRORS =======================================================
________________________________________ ERROR collecting anytree/__init__.py ________________________________________
anytree/__init__.py:18: in <module>
    from .node import AnyNode as AnyNode  # noqa
anytree/node/__init__.py:12: in <module>
    from .anynode import AnyNode as AnyNode  # noqa
anytree/node/anynode.py:7: in <module>
    from .nodemixin import NodeMixin
anytree/node/nodemixin.py:6: in <module>
    from typing import TYPE_CHECKING, Any, Generic, TypeGuard, TypeVar, Union, cast
E   ImportError: cannot import name 'TypeGuard' from 'typing' (/usr/lib/python3.8/typing.py)

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.

@CoolCat467
Copy link
Author

This is because TypeVar was added in Python 3.10

@iainelder
Copy link

iainelder commented Aug 4, 2024

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!

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.

4 participants