enable mypy linting on tests/ directory

[danieleades]

mypy currently ignores the tests/ directory. We should enable it.

@picnixz this is likely a job for you as you're far more comfortable with the sphinx tests than I!

This could also be done incrementally using the 'allow-list' approach i've been using to ratchet up the type linting strictness in the main package

[picnixz]

Haha clearly for me. I once run mypy on tests by mistake and I got so many errors that I gave up. It will be a long-standing task though.

[danieleades]

Haha clearly for me. I once run mypy on tests by mistake and I got so many errors that I gave up. It will be a long-standing task though.

definitely an incremental approach then!

as in, add all test modules to the allow list and then remove them one at a time

[picnixz]

as in, add all test modules to the allow list and then remove them one at a time

yes, that was the idea. I'll take care of this err... perhaps tomorrow (namely, making the white list, not fixing the errors)

[danieleades]

as in, add all test modules to the allow list and then remove them one at a time

yes, that was the idea. I'll take care of this err... perhaps tomorrow (namely, making the white list, not fixing the errors)

i had a quick look at this. I think if you could refactor and annotate the 'assert_node' method that alone would be a huge improvement. That is one ugly method.

[picnixz]

Ok bad news:

• assert_node is impossible to type properly. Reason is that document[0] is a Node and a Node is not indexable so you end up with errors about document[0] being non-indexable (so you cannot write document[0][1]). Many errors stem from this.
• another bad news: mypy cannot determine that

[nodes.table, nodes.tgroup, (nodes.colspec,
                                              nodes.colspec,
                                              nodes.colspec,
                                              [nodes.tbody, nodes.row])]

is actually a list[type[Node] | tuple[type[Node] | list[type[Node]], ...]] and instead derives it as a list[object]. So, ths second argument of assert_node can be typed a bit more but... it won't help us.

• So, docutils is actually winning this battle (it's right to assume that document[0] is only a node but most of the nodes are still indexable, if not all !).

• The last thing we could do have a mypy plugin which changes the signature of assert_node in a way that makes it compatible. Like, you would check that the first argument is indeed of type node and the second argument is indeed a good type (even though you need to unroll everything).

For now, I think we need to suppress every mypy error manually.. or just ignore them but it doesn't really solve our original problem =/

[danieleades]

it might not be that complicated:

Ok bad news:

• assert_node is impossible to type properly. Reason is that document[0] is a Node and a Node is not indexable so you end up with errors about document[0] being non-indexable (so you cannot write document[0][1]). Many errors stem from this.

Element is indexable, and many nodes are subclasses of Element. perhaps this should be typed as Element instead of Node if the method assumes it is indexable?

• another bad news: mypy cannot determine that

[nodes.table, nodes.tgroup, (nodes.colspec,
                                              nodes.colspec,
                                              nodes.colspec,
                                              [nodes.tbody, nodes.row])]

is actually a list[type[Node] | tuple[type[Node] | list[type[Node]], ...]] and instead derives it as a list[object]. So, ths second argument of assert_node can be typed a bit more but... it won't help us.

i'm not sure what you mean?

that type you've copied looks to me like a list[type[Node] | Sequence[type[Node] | Sequence[type[Node]]]]

• So, docutils is actually winning this battle (it's right to assume that document[0] is only a node but most of the nodes are still indexable, if not all !).
• The last thing we could do have a mypy plugin which changes the signature of assert_node in a way that makes it compatible. Like, you would check that the first argument is indeed of type node and the second argument is indeed a good type (even though you need to unroll everything).

a mypy plugin seems like overkill

For now, I think we need to suppress every mypy error manually.. or just ignore them but it doesn't really solve our original problem =/

[picnixz]

Element is indexable, and many nodes are subclasses of Element. perhaps this should be typed as Element instead of Node if the method assumes it is indexable?

The Element __getitem__ would still return a Node and not an Element. There is no easy fix for that I think.

that type you've copied looks to me like a list[type[Node] | Sequence[type[Node] | Sequence[type[Node]]]]

Yes but when you do a reveal_type with mypy, it's list[object]. So when you call assert_node, it tells me that the type is invalid.

[AA-Turner]

Is there work remaining here @danieleades? Clearly there are still modules in the blacklist, but does this issue need to remain open?

A

[danieleades]

Is there work remaining here @danieleades? Clearly there are still modules in the blacklist, but does this issue need to remain open?

A

Probably not