start fixing mypy issues #5772

graingert · 2025-04-27T08:28:11Z

starts work on #5773

this also builds on pre-commit being run #5717

Please review the following checklist.

Docstrings on all new or modified functions / classes
Updated documentation
Updated CHANGELOG.md (where appropriate)

willmcgugan

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.

willmcgugan · 2025-04-30T18:58:44Z

src/textual/_work_decorator.py

@@ -33,42 +33,41 @@ class WorkerDeclarationError(Exception):
    """An error in the declaration of a worker method."""


-if TYPE_CHECKING:


Why remove this? Given that the overloads are not needed at runtime, this check is a small win for startup time.

This is not supported by mypy, it needs the definition in the same block as the overrides

Just tried, and Mypy seems happy with the condition. Do we have different versions of Mypy?

I assumed I had the one from the poetry lock file, I'll check

I have 1.14.1

for example when I run mypy I get no-overload-impl for this patten.

mypy src/textual/walk.py src/textual/walk.py:88: error: An overloaded function outside a stub file must have an implementation [no-overload-impl] src/textual/walk.py:104: error: Name "walk_breadth_first" already defined on line 88 [no-redef] Found 2 errors in 1 file (checked 1 source file)

however not for this case:

mypy src/textual/_work_decorator.py src/textual/_work_decorator.py:71: error: Type variable "textual._work_decorator.ReturnType" is unbound [valid-type] src/textual/_work_decorator.py:71: note: (Hint: Use "Generic[ReturnType]" or "Protocol[ReturnType]" base class to bind "ReturnType" inside a class) src/textual/_work_decorator.py:71: note: (Hint: Use "ReturnType" in function signature to bind "ReturnType" inside a function)

willmcgugan · 2025-04-30T19:02:34Z

src/textual/dom.py

-        while (node := node._parent) is not None:
-            add_node(node)
-        return cast("list[DOMNode]", nodes)
+        nodes = [self]


This change will make this method slower. If it is to satisfy Mypy, can we not have both fast and correct?

the add_node = node.append trick is actually slower now

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.

willmcgugan · 2025-04-30T19:04:53Z

src/textual/document/_syntax_aware_document.py

-
-        captures = query.captures(self._syntax_tree.root_node, **captures_kwargs)
-        return captures
+


This change is more than typing. Can you explain why this change was necessary?

This is needed for the new interface in tree-sitter, it's a breaking change in v24

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.

willmcgugan · 2025-04-30T19:05:13Z

src/textual/dom.py

-        while (node := node._parent) is not None:
-            add_node(node)
-        return cast("list[DOMNode]", nodes)
+        nodes = []


Same comment as above.

willmcgugan · 2025-04-30T19:06:09Z

src/textual/drivers/_input_reader.py


-if WINDOWS:
+if sys.platform == "win32":


If we are going to do this, do we need the WINDOWS constant?

I suspect the WINDOWS constant is redundant

Suggest we remove it.

willmcgugan · 2025-04-30T19:14:32Z

src/textual/message_pump.py

@@ -243,7 +243,7 @@ def is_attached(self) -> bool:
        except NoActiveAppError:
            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:


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.

willmcgugan · 2025-04-30T19:15:57Z

src/textual/reactive.py

@@ -150,7 +151,7 @@ def __rich_repr__(self) -> rich.repr.Result:
        yield "name", getattr(self, "name", None), None

    @classmethod
-    def _clear_watchers(cls, obj: Reactable) -> None:
+    def _clear_watchers(cls, obj: MessagePump) -> None:


Given that a Reactable is a DOMNode, why is this change neccesary?

_clear_watchers is called with a less specific type than Reactable or DOMNode elsewhere

willmcgugan · 2025-04-30T19:17:06Z

src/textual/walk.py

@@ -17,22 +17,20 @@
    WalkType = TypeVar("WalkType", bound=DOMNode)


-if TYPE_CHECKING:


Same comment re overloads. They just add to startup time.

willmcgugan · 2025-04-30T19:18:29Z

src/textual/widget.py

@@ -979,15 +986,14 @@ def get_child_by_id(
            )
        return child

-    if TYPE_CHECKING:


Overloads again

willmcgugan · 2025-04-30T19:21:22Z

src/textual/worker.py

-            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]


Is this really better? Can't we modify the original in a simpler way to satisfy Mypy?

The type for the runner local is very complicated and would need to be explicitly stated here

graingert added 18 commits April 7, 2025 15:01

apply pre-commit

2ecf041

fix vcs permalink

87cc68d

pre-commit autoupdate

b3e0192

restore SVGs from main

5ecbf61

exclude svg from end of file fixer

945f418

Merge branch 'main' into fix-linting

98ce3f6

fix widgets.md

f5c221d

fix styles.md

5080010

Update styles.md

bfbcfb7

Merge branch 'main' into fix-linting

1c82183

Merge branch 'main' into fix-linting

7f0efca

Merge branch 'main' into fix-linting

c699d23

Merge branch 'main' into fix-linting

69ecdc9

first mypy fixes

d40d79c

update poetry lock

1d2ea2c

MessagePumps have Self as Parent

8bc76d2

upgrade typing_extensions

f5a884c

first 20 or so typing errors

4f206d1

graingert marked this pull request as ready for review April 27, 2025 08:30

willmcgugan reviewed Apr 30, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

start fixing mypy issues #5772

start fixing mypy issues #5772

graingert commented Apr 27, 2025 •

edited

Loading

willmcgugan left a comment

willmcgugan Apr 30, 2025

graingert Apr 30, 2025

willmcgugan May 1, 2025

graingert May 1, 2025

graingert May 1, 2025

willmcgugan Apr 30, 2025

graingert Apr 30, 2025

willmcgugan May 1, 2025

willmcgugan Apr 30, 2025

graingert Apr 30, 2025

willmcgugan May 1, 2025

willmcgugan Apr 30, 2025

willmcgugan Apr 30, 2025

graingert Apr 30, 2025

willmcgugan May 1, 2025

willmcgugan Apr 30, 2025

willmcgugan Apr 30, 2025

graingert Apr 30, 2025

willmcgugan Apr 30, 2025

willmcgugan Apr 30, 2025

willmcgugan Apr 30, 2025

graingert Apr 30, 2025

		@@ -33,42 +33,41 @@ class WorkerDeclarationError(Exception):
		"""An error in the declaration of a worker method."""


		if TYPE_CHECKING:


		captures = query.captures(self._syntax_tree.root_node, **captures_kwargs)
		return captures

		@@ -17,22 +17,20 @@
		WalkType = TypeVar("WalkType", bound=DOMNode)


		if TYPE_CHECKING:

start fixing mypy issues #5772

Are you sure you want to change the base?

start fixing mypy issues #5772

Conversation

graingert commented Apr 27, 2025 • edited Loading

willmcgugan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graingert commented Apr 27, 2025 •

edited

Loading