Skip to content

docutils: complete nodes.Node & bump version to 0.18.* #7380

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

Merged
merged 7 commits into from
Feb 28, 2022

Conversation

not-my-profile
Copy link
Contributor

The version bump is necessary because
Node.findall() was introduced in 0.18.1.

The version bump is necessary because
Node.findall() was introduced in 0.18.1.
@not-my-profile
Copy link
Contributor Author

not-my-profile commented Feb 25, 2022

question I had because I misread the error message

Passing a type as the condition to Node.findall is equivalent to lambda n: isinstance(n, type) ... hence the overload:

@overload
def findall(self, condition: type[_N], ...) -> Generator[_N, None, None]: ...
@overload
def findall(self, condition: Callable[[Node], bool] = ..., ...) -> Generator[Node, None, None]: ...

I intentionally omitted the = ... for condition in the first overload because otherwise, Pyright picks the first overload for node.findall() and infers _N@findall as a return type, which is obviously wrong.

Omitting the default value for the first overload however results in mypy.stubtest failing:

error: docutils.nodes.Node.findall is inconsistent, runtime argument "condition" has a default value of type None, which is incompatible with stub argument type Union[Type[_N`-1], def (docutils.nodes.Node) -> builtins.bool]. This is often caused by overloads failing to account for explicitly passing in the default value.

What do you suggest?

Inferring _N@findall for the return type shouldn't lead to type checking errors since _N is bounded by Node but I'd still prefer type checkers to infer Node as a return type for node.findall() because _N@findall might confuse users looking at the type tooltips in their IDE.

Sidenote: I unfortunately cannot check if mypy infers the same because of python/mypy#12241.

Copy link
Collaborator

@Akuli Akuli left a comment

Choose a reason for hiding this comment

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

Didn't read most of the PR yet, but this should answer your question.

@Akuli
Copy link
Collaborator

Akuli commented Feb 25, 2022

Sidenote: I unfortunately cannot check if mypy infers the same because of python/mypy#12241.

You can set the MYPYPATH environment variable:

(env) akuli@akuli-desktop:~/typeshed$ cat asd.py
from docutils.nodes import Element
e: Element
reveal_type(e.findall())

(env) akuli@akuli-desktop:~/typeshed$ MYPYPATH=stubs/docutils/ mypy asd.py
asd.py:3: note: Revealed type is "typing.Generator[docutils.nodes.Node, None, None]"

Copy link
Collaborator

@srittau srittau left a comment

Choose a reason for hiding this comment

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

Thanks, a few notes below.

@srittau srittau merged commit 98d9ed4 into python:master Feb 28, 2022
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.

5 participants