start fixing mypy issues#5772
Conversation
willmcgugan
left a comment
willmcgugan
left a comment
There was a problem hiding this comment.
I'm not keen on changes which make things slower purely to satisfy the type checker.
One-off asserts or casts I can accept. But I'd prefer to avoid code within loops that isn't required for runtime.
| while (node := node._parent) is not None: | ||
| add_node(node) | ||
| return cast("list[DOMNode]", nodes) | ||
| nodes = [self] |
There was a problem hiding this comment.
This change will make this method slower. If it is to satisfy Mypy, can we not have both fast and correct?
There was a problem hiding this comment.
the add_node = node.append trick is actually slower now
There was a problem hiding this comment.
Last time I looked in to this, nodes.append version was still slower. Although the difference is much smaller in recent Pythons.
The node is not None also adds work, and I doesn't appear to be neccesary.
There was a problem hiding this comment.
I've looked into this and have benchmarked some options:
(there's not all that much in it, even if you really crank up the number of nodes)
https://gist.github.com/graingert/23d43b7fb7c8d5663cc355e8390f19d9
the option I want to go for is:
nodes = []
node: DOMNode | None = self
while True:
if node is None:
return nodes
nodes.append(node)
node = node._parentsome remarks: repeatedly looking up .append is actually much faster, doing the condition in the body of a while loop is faster this is probably a bug!
|
|
||
| captures = query.captures(self._syntax_tree.root_node, **captures_kwargs) | ||
| return captures | ||
|
|
There was a problem hiding this comment.
This change is more than typing. Can you explain why this change was necessary?
There was a problem hiding this comment.
This is needed for the new interface in tree-sitter, it's a breaking change in v24
There was a problem hiding this comment.
The latest tree-sitter also drops support for Python3.9, which Textual hasn't yet. We've already decided to drop syntax support for 3.9, but we would need to update the pyproject.toml accordingly.
| while (node := node._parent) is not None: | ||
| add_node(node) | ||
| return cast("list[DOMNode]", nodes) | ||
| nodes = [] |
| WINDOWS: Final = sys.platform == "win32" | ||
|
|
||
| if WINDOWS: | ||
| if sys.platform == "win32": |
There was a problem hiding this comment.
If we are going to do this, do we need the WINDOWS constant?
There was a problem hiding this comment.
I suspect the WINDOWS constant is redundant
| return False | ||
| node: MessagePump | None = self | ||
| while (node := node._parent) is not None: | ||
| while node is not None and (node := node._parent) is not None: |
There was a problem hiding this comment.
This adds an additional check per loop, which is never going to be truthy. I think we should find another way to satisfy Mypy if that is why this was added.
|
|
||
| @classmethod | ||
| def _clear_watchers(cls, obj: Reactable) -> None: | ||
| def _clear_watchers(cls, obj: MessagePump) -> None: |
There was a problem hiding this comment.
Given that a Reactable is a DOMNode, why is this change neccesary?
There was a problem hiding this comment.
_clear_watchers is called with a less specific type than Reactable or DOMNode elsewhere
| WalkType = TypeVar("WalkType", bound=DOMNode) | ||
|
|
||
|
|
||
| if TYPE_CHECKING: |
There was a problem hiding this comment.
Same comment re overloads. They just add to startup time.
| ) | ||
| return child | ||
|
|
||
| if TYPE_CHECKING: |
| runner = run_callable | ||
| else: | ||
| raise WorkerError("Unsupported attempt to run a thread worker") | ||
| return await loop.run_in_executor(None, run_coroutine, self._work) # type: ignore[arg-type] |
There was a problem hiding this comment.
Is this really better? Can't we modify the original in a simpler way to satisfy Mypy?
There was a problem hiding this comment.
The type for the runner local is very complicated and would need to be explicitly stated here
2 participants
starts work on #5773
this also builds on pre-commit being run #5717
Please review the following checklist.