Make mypy happy #1831

rodrigogiraoserrao · 2023-02-17T23:51:51Z

This is a draft because I'm still axing issues, but each commit corresponds to one fix (either a single problem or tightly related problems).

Feel free to review each commit individually and let me know if/where I dropped the ball.

Fixes #1805

@willmcgugan

@willmcgugan I went with the asserts because the tone of the code tells me that we are kind of certain that the current and previous frames always exist. Should the function be refactored to handle None for the previous frame (or the current & previous frames) by writing to the buffer without caller information, for example?

Looks like the two shared a lot of attributes, so we were trying to use FrameInfo but inspect.getframeinfo returns a Traceback, so that felt like the correct type to use.

Same fix as in b709219.

There was another alternative solution, which was to just flatten everything entirely, so that the code actually obeyed the comments. After all, the comments for the attribute `order` in the definition of `MapGeometry` said that `order` was a tuple of integers defining the painting order, which was not true because the very first widget was having its order defined as `order = ((0,),)` instead of `order = (0,)`. Thus, another possible solution would be to retype `order` as `tuple[int, ...]` and make everything flat, but I opted for the “tuple of tuples” approach because the sub-tuples will highlight the nested structure of the DOM hierarchy.

[skip cli]

rodrigogiraoserrao · 2023-02-20T13:58:56Z

eeeac1a fixes the typing of fromkeys with

fromkeys = cast("Callable[[list[int]], dict[int, Strip | None]]", dict.fromkeys)
chops: list[dict[int, Strip | None]]
chops = [fromkeys(cut_set[:-1]) for cut_set in cuts]

I feel like a purer solution would be to use a dictionary comprehension:

chops: list[dict[int, Strip | None]]
chops = [{key: None for key in cut_set[:-1]} for cut_set in cuts]

This seems to be around 10%-15% slower:

>>> timeit.repeat("{k: None for k in range(5)}")
[0.2364686660002917, 0.2092778330006695, 0.20847449999928358, 0.20909275000121852, 0.20904904100098065]
>>> timeit.repeat("dict.fromkeys(range(5))")
[0.20496962499964866, 0.1807063329997618, 0.1800928329994349, 0.1798485420003999, 0.17970204199991713]
>>> timeit.repeat("{k: None for k in range(500)}", number=10_000)
[0.1311742920006509, 0.10960308299945609, 0.10958245799884025, 0.11009866600034002, 0.10938712500137626]
>>> timeit.repeat("dict.fromkeys(range(500))", number=10_000)
[0.112881875000312, 0.08676441699935822, 0.0867214169993531, 0.08729300000049989, 0.08724679199985985]

Two other alternatives would be: - leave typing of 'widget: DOMNode' and then assert 'widget' is actually of type 'Widget', which works just fine but looks weird in the sense that we type a parameter in one way but then only manage to do any work if it is of another type; - type it as 'widget: DOMNode | Widget' and set 'size' to something other than 'widget.outer_size' if 'widget' is a 'DOMNode'.

Adding an 'assert not isinstance(spacing, int)' before raising the error sounds reasonable, because 'Spacing.unpack' seems to be able to handle a lone integer just fine, but I decided against it because I would not like to raise an assertion error from within an exception handling inside Textual. So, to keep it on the safer side, I went with the conditional expression that checks if we have an integer or a tuple.

The obvious fix would be to do 'default_factory=RulesMap' but mypy will infer that the type of 'RulesMap' is 'type[RulesMap]' and will not see it as a 'Callable[[], RulesMap]'. That could be fixed by using 'cast'. I decided to use 'RulesMap.__call__'. [skip ci]

We fix the LSP violation by using the abstract methods that the ABC already provides... Which is a shame, because I thought I was about to write my first Protocol.

rodrigogiraoserrao · 2023-02-20T18:20:05Z

TODO: Go back to 0ae4633 and see if I should raise an error if token is None. Given the context in which we're in (because token.name == "variable_name" in the outer scope), we probably can't get away without raising an error if there are no tokens left.

src/textual/_compositor.py

This reverts commit 0ae4633. Upon closer look, this is not the correct fix.

Checking if the token is not 'None' brings us a tiny step closer to fixing #1849, which still needs to ensure the variable definition is complete and valid, even if empty.

After some fiddling, some crying, and talking to Dave and Will, we got to a partial solution. I cried a bit more and came up with the fix that entailed lifting 'ExpectType' to outside of 'DOMNode'. Then, I realised 'ExpectType' and 'QueryType' from 'query.py' were essentially the same thing so I decided to only make use of 'QueryType'.

mypy can't infer that if after is None, then before won't.

@willmcgugan

@willmcgugan did I understand correctly that this 'cast' is exactly what renderable being possibly a 'RichCast' asks me to do..? To be honest, I was not 100% sure after reading rich's documentation for 'RichCast' and after reading the source of 'rich_cast'.

Fixes: #1870.

rodrigogiraoserrao · 2023-02-24T12:23:07Z

On Python 3.7, 9 issues remain.
On Python 3.11, only 2.

This change follows from a discussion in issue #1871.

willmcgugan

A few comments for your consideration.

You may have gone a little bit beyond the remit of "fix typing errors". Might have been better to create issues for the larger problems you identified rather than fix them here.

But great stuff, you solved a number of irritating typing issues we've had for a while.

willmcgugan · 2023-02-26T20:57:18Z

src/textual/_compositor.py

-            "Callable[[list[int]], dict[int, list[Segment] | None]]", dict.fromkeys
-        )
-        # A mapping of cut index to a list of segments for each line
+        fromkeys = cast("Callable[[list[int]], dict[int, Strip | None]]", dict.fromkeys)


Comment above needs updating.

The diff isn't great, but the comment is correct: it's "# dict.fromkeys is a callable [...]" and is the old line 821 / the new line 837...

The comment references list of Segments or None, but the annotation references Strip | None

src/textual/_compositor.py

src/textual/actions.py

src/textual/app.py

src/textual/css/styles.py

src/textual/demo.py

willmcgugan · 2023-02-26T21:41:53Z

src/textual/keys.py

@@ -197,6 +196,18 @@ class Keys(str, Enum):
    ShiftControlEnd = ControlShiftEnd


+# We want to make sure that mypy knows that the values of Keys will always be strings.


I don't know what is going on here.

Can you explain it like I'm 5?

Fast-forward: This was the very first typing issue I tried to fix (I tackled them in alphabetical order 😅 ) and I couldn't figure out anything better.

TL;DR: the typing of Enum.value in enum.pyi is very meta because of the way the attribute value is implemented. When trying to solve this issue, I focused too much on what the typing of value looked like in enum.pyi and forgot to think about the actual implementation of value in enum.py.
Taking a look at it now inspired a simpler solution.

It doesn't feel like the right way to do things. The fact that you have to ignore the redefinition on 205 makes me suspicious.

Think it might be better to revert this one, and we will tackle it in the future when I have time to grok it.

Sorry, I forgot to commit and push the newest fix that looks much more reasonable.
I'll wait for your feedback.

willmcgugan · 2023-02-26T21:50:51Z

src/textual/widgets/_data_table.py

@@ -64,18 +64,37 @@ class DuplicateKey(Exception):
    an existing row or column in the DataTable. Keys must be unique."""


+StringKeySubclass = TypeVar("StringKeySubclass", bound="StringKey")


@darrenburns Could you look over Rodrigo's changes in this file please?

willmcgugan · 2023-02-26T21:54:13Z

src/textual/widgets/_data_table.py

@@ -138,6 +157,53 @@ def __rich_repr__(self):
        yield "column_key", self.column_key


+class KeysToIndices(TwoWayDict[StringKeySubclass, int]):


What is the purpose of this class over a plain TwoWayDict? Is it to generate a more specific exception? Was there a specific case were the DataTable was generating a non specific exception?

TwoWayDict was lenient and allowed returning Nones from its get methods.
When that was correctly typed, many typing errors appeared because the code does not contemplate the possibility of getting Nones back from its internal bidirectional dictionaries.

I started tackling those one by one, doing a runtime check for None and raising the appropriate error.

As I did that, I realised the code was getting bloated with all those checks.
So, I decided to lift those checks to the specialised bidirectional dictionary KeysToIndices that is responsible for raising the appropriate errors when fetching a key or a value fails.
(Which the semantics of the data table code imply should never happen.)

I thought this was a good solution but I can be convinced otherwise.

If there are no plans to use the general bidirectional dictionary elsewhere, we can merge TwoWayDict and KeysToIndices into a single class.

I have a suspicion that the DataTable code checks for these conditions, so the TWD never returns None at runtime.

Perhaps the get and get_key should just use [] syntax.

@darrenburns Can you confirm?

I just merged main and I have a couple of failing tests because I'm getting Nones from get/get_key.

This reverts commit f366783.

Related comments: https://github.com/Textualize/textual/pull/1831\#discussion_r1118155296

See: https://github.com/Textualize/textual/pull/1831\#discussion_r1118154777

willmcgugan · 2023-03-01T13:41:48Z

@rodrigogiraoserrao I'd like to get this merged soon. Could you revert the changes to DataTable please. We can consider the DT stuff for a future update, but I'd like to have Darren look over them when he's back in the office (he's sick ATM).

rodrigogiraoserrao · 2023-03-01T15:12:18Z

@rodrigogiraoserrao I'd like to get this merged soon. Could you revert the changes to DataTable please. We can consider the DT stuff for a future update, but I'd like to have Darren look over them when he's back in the office (he's sick ATM).

Changes reverted.

rodrigogiraoserrao · 2023-03-01T15:13:22Z

This PR will not knock down all existing typing issues.

Should we leave issue #1822 (Fix typing) open or should I open a new issue that is more specific?

willmcgugan · 2023-03-01T15:28:52Z

Thanks. I think we can leave the Fix Typing PR open. It will be an ongoing thing.

rodrigogiraoserrao · 2023-03-01T15:40:37Z

Thanks. I think we can leave the Fix Typing PR open. It will be an ongoing thing.

Ok. In that case, I just need someone to approve this PR. (Maybe this one should be squash-merged.)

Works around a bug that was introduced in Textualize/textual#1831. Closes #3.

rodrigogiraoserrao added 5 commits February 17, 2023 18:46

Helper file for error progress tracking.

7aa201e

Fix typing of Keys.???.value.

e93b7e2

Use inspect.Traceback instead of inspect.FrameInfo.

aad3660

Looks like the two shared a lot of attributes, so we were trying to use FrameInfo but inspect.getframeinfo returns a Traceback, so that felt like the correct type to use.

Update after installing msgpack-types.

148ce03

rodrigogiraoserrao force-pushed the make-mypy-happy branch from 6261616 to 148ce03 Compare February 17, 2023 23:55

rodrigogiraoserrao added 4 commits February 20, 2023 09:38

Assert we have frame info.

a410afa

Same fix as in b709219.

Fix import and typing for fromkeys.

eeeac1a

Assert app is not None.

cb091a3

[skip cli]

rodrigogiraoserrao added 10 commits February 20, 2023 14:54

Import missing type.

2f9421d

Use CallbackType for event Callback.

ef8f46b

Remove variable name clash.

4795910

Use correct default.

4d60907

The obvious fix would be to do 'default_factory=RulesMap' but mypy will infer that the type of 'RulesMap' is 'type[RulesMap]' and will not see it as a 'Callable[[], RulesMap]'. That could be fixed by using 'cast'. I decided to use 'RulesMap.__call__'. [skip ci]

Use correct abstract methods.

83356ed

We fix the LSP violation by using the abstract methods that the ABC already provides... Which is a shame, because I thought I was about to write my first Protocol.

Add missing annotation.

e297c8c

Fix type inference.

0c52f70

Check token is not None.

0ae4633

willmcgugan reviewed Feb 20, 2023

View reviewed changes

src/textual/_compositor.py Show resolved Hide resolved

rodrigogiraoserrao added 7 commits February 21, 2023 11:26

Revert "Check token is not None."

c84a3c4

This reverts commit 0ae4633. Upon closer look, this is not the correct fix.

Check that token is not None.

4cea32d

Checking if the token is not 'None' brings us a tiny step closer to fixing #1849, which still needs to ensure the variable definition is complete and valid, even if empty.

Infer correct type while walking.

53d49f4

Cast to remove doubt about None.

870e71b

mypy can't infer that if after is None, then before won't.

Explicitly type variable.

54f9226

Cast to console renderable.

8ae1665

@willmcgugan did I understand correctly that this 'cast' is exactly what renderable being possibly a 'RichCast' asks me to do..? To be honest, I was not 100% sure after reading rich's documentation for 'RichCast' and after reading the source of 'rich_cast'.

rodrigogiraoserrao marked this pull request as ready for review February 23, 2023 19:49

rodrigogiraoserrao requested review from willmcgugan, darrenburns and davep February 23, 2023 19:49

rodrigogiraoserrao added 2 commits February 24, 2023 11:14

Fix Python compat. issues.

336f000

Button can only accept str/Text as label.

a4072c4

Fixes: #1870.

rodrigogiraoserrao force-pushed the make-mypy-happy branch from 74efdc1 to a4072c4 Compare February 24, 2023 12:22

rodrigogiraoserrao linked an issue Feb 24, 2023 that may be closed by this pull request

Button rendering fails for the general label case #1870

Closed

Add runtime type check for list items.

aadfd0d

This change follows from a discussion in issue #1871.

willmcgugan reviewed Feb 26, 2023

View reviewed changes

rodrigogiraoserrao mentioned this pull request Feb 27, 2023

[Typing] Add type hint to sender in widget messages? #1813

Closed

rodrigogiraoserrao added 5 commits February 27, 2023 12:18

Address review comments.

8675639

Revert "Fix typing issues in demo."

dccf167

This reverts commit f366783.

Address review comments.

fa5d6cf

Related comments: https://github.com/Textualize/textual/pull/1831\#discussion_r1118155296

Add clarifying comment.

865ef14

See: https://github.com/Textualize/textual/pull/1831\#discussion_r1118154777

Merge branch 'main' into make-mypy-happy

cb612c0

rodrigogiraoserrao added 2 commits March 1, 2023 15:10

Revert changes to data table.

db3372d

Merge branch 'main' into make-mypy-happy

844d9c1

rodrigogiraoserrao removed a link to an issue Mar 1, 2023

Fix typing for MyPy #1822

Closed

willmcgugan approved these changes Mar 1, 2023

View reviewed changes

willmcgugan merged commit ed28a70 into main Mar 1, 2023

willmcgugan deleted the make-mypy-happy branch March 1, 2023 15:50

dcambie mentioned this pull request Mar 9, 2023

ImportError: cannot import name 'Any' from 'typing_extensions' #1995

Closed

yasyf added a commit to yasyf/summ that referenced this pull request Mar 16, 2023

Restore sys.stderr so langchain does not explode.

592b42a

Works around a bug that was introduced in Textualize/textual#1831. Closes #3.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make mypy happy #1831

Make mypy happy #1831

rodrigogiraoserrao commented Feb 17, 2023 •

edited by willmcgugan

Loading

rodrigogiraoserrao commented Feb 20, 2023

rodrigogiraoserrao commented Feb 20, 2023

rodrigogiraoserrao commented Feb 24, 2023

willmcgugan left a comment

willmcgugan Feb 26, 2023

rodrigogiraoserrao Feb 27, 2023

willmcgugan Feb 27, 2023

willmcgugan Feb 26, 2023

rodrigogiraoserrao Feb 27, 2023

willmcgugan Feb 27, 2023

rodrigogiraoserrao Feb 27, 2023 •

edited

Loading

willmcgugan Feb 26, 2023

willmcgugan Feb 26, 2023

rodrigogiraoserrao Feb 27, 2023

willmcgugan Feb 27, 2023

rodrigogiraoserrao Feb 27, 2023

willmcgugan commented Mar 1, 2023

rodrigogiraoserrao commented Mar 1, 2023

rodrigogiraoserrao commented Mar 1, 2023

willmcgugan commented Mar 1, 2023

rodrigogiraoserrao commented Mar 1, 2023

		@@ -197,6 +196,18 @@ class Keys(str, Enum):
		ShiftControlEnd = ControlShiftEnd


		# We want to make sure that mypy knows that the values of Keys will always be strings.

		@@ -64,18 +64,37 @@ class DuplicateKey(Exception):
		an existing row or column in the DataTable. Keys must be unique."""


		StringKeySubclass = TypeVar("StringKeySubclass", bound="StringKey")

		@@ -138,6 +157,53 @@ def __rich_repr__(self):
		yield "column_key", self.column_key


		class KeysToIndices(TwoWayDict[StringKeySubclass, int]):

Make mypy happy #1831

Make mypy happy #1831

Conversation

rodrigogiraoserrao commented Feb 17, 2023 • edited by willmcgugan Loading

rodrigogiraoserrao commented Feb 20, 2023

rodrigogiraoserrao commented Feb 20, 2023

rodrigogiraoserrao commented Feb 24, 2023

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

rodrigogiraoserrao Feb 27, 2023 • edited Loading

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

willmcgugan commented Mar 1, 2023

rodrigogiraoserrao commented Mar 1, 2023

rodrigogiraoserrao commented Mar 1, 2023

willmcgugan commented Mar 1, 2023

rodrigogiraoserrao commented Mar 1, 2023

rodrigogiraoserrao commented Feb 17, 2023 •

edited by willmcgugan

Loading

rodrigogiraoserrao Feb 27, 2023 •

edited

Loading