Skip to content

Conversation

@danieleades
Copy link
Contributor

@danieleades danieleades commented Mar 2, 2024

Improve type safety by refactoring the NodeMatcher.

Mypy is unable to infer the return type of the Node.find_all method when NodeMatcher is used. By moving the helper method into the NodeMatcher class and adding some generics it's possible for type checkers to correctly infer the return type of the iterator.

I've noticed that a matcher rarely gets reused, so an alternative approach would be to get rid of the helper class entirely and turn it into a plain function.

@danieleades danieleades marked this pull request as ready for review March 2, 2024 13:08
@danieleades danieleades marked this pull request as draft March 2, 2024 13:25
@danieleades danieleades marked this pull request as ready for review March 2, 2024 14:02
@jayaddison
Copy link
Contributor

Are any of the affected callsites recursive methods?

What I'm thinking: an additional frame/entry on the stack (for the intermediate method call) is usually likely to have negligible effect for a call-and-return, but within a deeply recursive method it could have a significant effect on memory usage.

@danieleades danieleades requested a review from picnixz March 2, 2024 14:36
Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

Seems fine to me but can you confirm that self.document is of correct type actually ? (what is the revealed type?) because I remember that mypy was not happy sometimes because of the fact that self.document was not typed at all (and did not exist on SphinxTransform).

@danieleades
Copy link
Contributor Author

Seems fine to me but can you confirm that self.document is of correct type actually ? (what is the revealed type?) because I remember that mypy was not happy sometimes because of the fact that self.document was not typed at all (and did not exist on SphinxTransform).

i'm making some changes to the type stubs upstream in typeshed that should help with this

@picnixz picnixz merged commit bea8b6b into sphinx-doc:master Mar 2, 2024
@picnixz
Copy link
Member

picnixz commented Mar 2, 2024

Thank you!

@jayaddison
Copy link
Contributor

Are any of the affected callsites recursive methods?

From a relatively quick check of the code: no, none of the affected methods are recursive.

@danieleades danieleades deleted the NodeMatcher branch March 2, 2024 15:44
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 6, 2024
@AA-Turner AA-Turner added this to the 7.3.0 milestone Jul 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants