anytree icon indicating copy to clipboard operation
anytree copied to clipboard

Add type annotations for nodes

Open CoolCat467 opened this issue 1 year ago • 15 comments

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

CoolCat467 avatar Jan 18 '24 01:01 CoolCat467

What is missing for this PR to be merged?

moisesluza avatar May 30 '24 22:05 moisesluza

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.

CoolCat467 avatar May 31 '24 01:05 CoolCat467

@c0fec0de could you please take a look?

moisesluza avatar May 31 '24 13:05 moisesluza

@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.

iainelder avatar Jul 30 '24 22:07 iainelder

Yes, yes it does

CoolCat467 avatar Jul 30 '24 22:07 CoolCat467

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 

iainelder avatar Jul 31 '24 13:07 iainelder

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 avatar Jul 31 '24 15:07 CoolCat467

Should be fixed with a182903

CoolCat467 avatar Aug 01 '24 05:08 CoolCat467

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.

iainelder avatar Aug 02 '24 07:08 iainelder

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.

CoolCat467 avatar Aug 02 '24 15:08 CoolCat467

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.

iainelder avatar Aug 02 '24 18:08 iainelder

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

CoolCat467 avatar Aug 02 '24 19:08 CoolCat467

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.

iainelder avatar Aug 03 '24 09:08 iainelder

This is because TypeVar was added in Python 3.10

CoolCat467 avatar Aug 03 '24 22:08 CoolCat467

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!

iainelder avatar Aug 04 '24 09:08 iainelder