Use mypy `strict` by Viicos · Pull Request #2180 · Kludex/starlette

Viicos · 2023-06-10T08:33:21Z

Fixes #2153.

mypy config updated:

no_implicit_optional is now the default (and seems to have been replaced by implicit_optional)
disallow_untyped_defs is included by strict

Checklist

I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
I've updated the documentation accordingly.

starlette/config.py

starlette/middleware/wsgi.py

adriangb

Looks good! Just a few small comments

starlette/concurrency.py

starlette/config.py

starlette/middleware/__init__.py

starlette/middleware/wsgi.py

adriangb · 2023-06-10T16:47:57Z

Please fix errors

Viicos · 2023-06-10T16:50:48Z

Please fix errors

This is still WIP, I'm doing it in small parts to avoid having a single big commit

adriangb · 2023-06-10T16:52:05Z

Ok no worries, I’ll just remove the approval until you’re ready

adriangb

Removing approval until this is marked as ready for review

Viicos · 2023-06-10T16:53:31Z

starlette/datastructures.py

-            "ImmutableMultiDict",
-            typing.Mapping,
+            "ImmutableMultiDict[typing.Any, typing.Any]",
+            typing.Mapping[typing.Any, typing.Any],


According to the signature of the ImmutableMultiDict's __init__ method:

class ImmutableMultiDict(typing.Mapping[_KeyType, _CovariantValueType]): _dict: typing.Dict[_KeyType, _CovariantValueType] def __init__( self, *args: typing.Union[ "ImmutableMultiDict[_KeyType, _CovariantValueType]", typing.Mapping[_KeyType, _CovariantValueType], typing.Iterable[typing.Tuple[_KeyType, _CovariantValueType]], ], **kwargs: typing.Any, ) -> None:

We should parametrize these ones as [str, str], but the following happens a few lines underneath:

self._list = [(str(k), str(v)) for k, v in self._list] self._dict = {str(k): str(v) for k, v in self._dict.items()}

starlette/middleware/errors.py

Viicos · 2023-06-10T19:54:11Z

starlette/templating.py

-        directory: typing.Union[
-            str, PathLike, typing.Sequence[typing.Union[str, PathLike]]
-        ],
+        directory: "typing.Union[str, PathLike[typing.AnyStr], typing.Sequence[typing.Union[str, PathLike[typing.AnyStr]]]]",


Is PathLike[AnyStr] fine? Or we could enforce str paths. I often see PathLike[Any] but doesn't feel right to me as PathLike is generic wrt TypeVar("AnyStr_co", str, bytes, covariant=True)

Kludex · 2023-06-20T05:52:57Z

@Viicos Do you need any help here?

Viicos · 2023-06-20T07:38:46Z

Hi @Kludex thanks for asking. The main blocking point is handling callables and coroutines together, as stated in #2180 (comment). The solution I provide should work but isn't pretty, and will add a lot of overhead to the code (considering this pattern is used a lot in the starlette codebase). Otherwise we could use Any as a return type, it seems to work.

Apart from that I'll be able to fix the remaining mypy issues

Kludex · 2023-06-20T18:40:45Z

Hi @Kludex thanks for asking. The main blocking point is handling callables and coroutines together, as stated in #2180 (comment). The solution I provide should work but isn't pretty, and will add a lot of overhead to the code (considering this pattern is used a lot in the starlette codebase). Otherwise we could use Any as a return type, it seems to work.

Apart from that I'll be able to fix the remaining mypy issues

Any is good for now. If you make the pipeline to pass I can review it. 👍

Viicos · 2023-06-20T20:26:49Z

Any is good for now. If you make the pipeline to pass I can review it. 👍

Will be ready for review by the end of the week, as I can see 1.0.0 is coming soon

starlette/concurrency.py

Kludex · 2023-06-22T07:15:05Z

Looks like the overrides from mypy is not working properly? 🤔

Viicos · 2023-06-23T20:26:38Z

~~Mypy errors left:~~

starlette/testclient.py:383: error: Name "httpx._client.CookieTypes" is not defined  [name-defined]
...
starlette/testclient.py:699: error: Name "httpx._client.TimeoutTypes" is not defined  [name-defined]

~~I have no idea why they happen~~ -> 8811c1c

Looks like the overrides from mypy is not working properly? 🤔

https://github.com/encode/starlette/blob/1be57fdb42522552f1e579912a22faa3088e1981/pyproject.toml#L68-L71

This one has no effect, module isn't working with a plain directory, instead it needs a proper module to import. For reference: python/mypy#10045
This doesn't seem trivial

Kludex · 2023-06-30T09:50:16Z

starlette/authentication.py

    redirect: typing.Optional[str] = None,
-) -> typing.Callable[[_CallableType], _CallableType]:
+) -> typing.Callable[
+    [typing.Callable[_P, typing.Any]], typing.Callable[_P, typing.Any]


Any reason for not using the following?

Suggested change

[typing.Callable[_P, typing.Any]], typing.Callable[_P, typing.Any]

[typing.Callable[_P, _T]], typing.Callable[_P, _T]

Couldn't make it work as it can be async or sync (same issue as #2180 (comment)) :/

starlette/types.py

starlette/testclient.py

starlette/staticfiles.py

Kludex · 2023-06-30T10:11:25Z

starlette/routing.py

        self,
        path: str,
-        endpoint: typing.Callable,
+        endpoint: typing.Callable[..., typing.Any],


Isn't an endpoint always Callable[[Request], typing.Any]?

I went for Callable[..., Any] as this endpoint argument is passed to routing.Route, which is doing the following:

https://github.com/encode/starlette/blob/2168e47052239da5df35d5353bb986f760c51cef/starlette/routing.py#L209-L234

So it seems endpoint can also be a ASGIApp (+ there's a bunch of things related to functools.partial)

Kludex · 2023-06-30T10:11:46Z

starlette/routing.py

-        self, path: str, endpoint: typing.Callable, name: typing.Optional[str] = None
+        self,
+        path: str,
+        endpoint: typing.Callable[..., typing.Any],


Isn't an endpoint always Callable[[WebSocket], typing.Any]?

See #2180 (comment) (same for #2180 (comment), #2180 (comment) if I'm not mistaken)

Kludex · 2023-06-30T10:12:52Z

starlette/routing.py

-        self, path: str, endpoint: typing.Callable, *, name: typing.Optional[str] = None
+        self,
+        path: str,
+        endpoint: typing.Callable[..., typing.Any],


Same as comments below.

Kludex · 2023-06-30T10:12:59Z

starlette/routing.py

        self,
        path: str,
-        endpoint: typing.Callable,
+        endpoint: typing.Callable[..., typing.Any],


Same as comments below.

starlette/requests.py

Kludex · 2023-06-30T10:14:22Z

starlette/middleware/errors.py

+        handler: typing.Optional[
+            typing.Callable[[Request, Exception], typing.Any]
+        ] = None,


Isn't this supposed to be ExceptionHandler as well? It's a question, not a suggestion.

Yes it is, this was discussed earlier: #2180 (comment). We could keep it as Any, or use Union[Reponse, Awaitable[Response]] and then add some casts or type: ignore

Kludex · 2023-07-06T18:02:50Z

Suggestions on what to do here? 😅

It looks like we are blocked by python/mypy#10045?

Viicos · 2023-07-06T19:06:30Z

It looks like we are blocked by python/mypy#10045?

I guess so. Maybe we could edit the check scripts to run mypy a second time on the tests directory, but I don't know if it will work as excepted.

Kludex · 2023-07-13T08:00:26Z

I've asked on the python/typing Gitter to see if someone has a good alternative. 👀

Viicos · 2023-07-13T08:02:36Z

In the meanwhile, I'll see if using a Union is feasible for #2180 (comment)

Viicos · 2023-07-16T19:58:06Z

@Kludex let me know if the check script tweak is ok 🙂

pyproject.toml

scripts/check

Kludex

Thanks @Viicos 🙏

Because his request for change was actually a dismissal of his previous approval

Viicos commented Jun 10, 2023

View reviewed changes

starlette/config.py Outdated Show resolved Hide resolved

Viicos commented Jun 10, 2023

View reviewed changes

starlette/middleware/wsgi.py Show resolved Hide resolved

adriangb suggested changes Jun 10, 2023

View reviewed changes

starlette/concurrency.py Outdated Show resolved Hide resolved

starlette/config.py Outdated Show resolved Hide resolved

starlette/middleware/__init__.py Show resolved Hide resolved

starlette/middleware/wsgi.py Show resolved Hide resolved

adriangb approved these changes Jun 10, 2023

View reviewed changes

adriangb previously requested changes Jun 10, 2023

View reviewed changes

Viicos commented Jun 10, 2023

View reviewed changes

starlette/middleware/errors.py Outdated Show resolved Hide resolved

Viicos commented Jun 10, 2023

View reviewed changes

Kludex added the typing Type annotations or mypy issues label Jun 20, 2023

Kludex added this to the Version 1.0 milestone Jun 20, 2023

Kludex reviewed Jun 20, 2023

View reviewed changes

starlette/concurrency.py Outdated Show resolved Hide resolved

Viicos marked this pull request as ready for review June 23, 2023 20:30

Kludex requested changes Jun 30, 2023

View reviewed changes

Kludex mentioned this pull request Jul 6, 2023

Version 0.29.0 #2209

Merged

3 tasks

Viicos force-pushed the 2153-mypy-strict branch from 8811c1c to 66c02f9 Compare July 16, 2023 18:27

Kludex requested changes Jul 16, 2023

View reviewed changes

pyproject.toml Outdated Show resolved Hide resolved

scripts/check Show resolved Hide resolved

Viicos and others added 14 commits July 23, 2023 08:38

Types: Fix last errors

5839e00

Format

9b2d75f

Types: fix pipeline

0eec96c

Format

a17681c

Ruff

a2689a3

Resolve circular import

e48f7ce

Fix usage of Sequence

f2cc766

Fix usage of MutableMapping

e32876e

Unused import

797eb9a

Apply feedback

adea150

improve comment on pyproject

46da1bd

Fix type hint of ExceptionHandler

72844ca

Use str for Environ

139d88a

Lint

6ec2072

Kludex force-pushed the 2153-mypy-strict branch from 2c67111 to 6ec2072 Compare July 23, 2023 06:38

Fix some inconsistencies

b65d473

Kludex force-pushed the 2153-mypy-strict branch from 381f83f to b65d473 Compare July 23, 2023 21:00

Kludex added 3 commits July 23, 2023 23:04

Fix 3.10

273fd1a

Use the right type on endpoint

0c6afb9

Simplify code source on applications.py

8b97b10

Kludex force-pushed the 2153-mypy-strict branch from 0545b0a to 8b97b10 Compare July 23, 2023 21:19

Kludex added 2 commits July 23, 2023 23:28

Fix is_async_callable type hint

f085e44

Fix more inconsistencies

d71287f

Kludex approved these changes Jul 23, 2023

View reviewed changes

Kludex merged commit 7c8ca17 into Kludex:master Jul 23, 2023

ravi-kumar-pilla mentioned this pull request Jan 4, 2024

Set Kedro Viz start method to spawn process while launching it from Jupyter Notebook kedro-org/kedro-viz#1696

Merged

5 tasks

severo mentioned this pull request Feb 5, 2024

upgrade starlette everywhere huggingface/dataset-viewer#2401

Merged

eggplants mentioned this pull request Feb 11, 2024

mypy error: type of _rate_limit_exceeded_handler is incompatible with latest starlette laurentS/slowapi#188

Open

patrick91 mentioned this pull request Jul 3, 2024

Using the ASGI app as a Starlette route throws typing errors strawberry-graphql/strawberry#3545

Open

	[typing.Callable[_P, typing.Any]], typing.Callable[_P, typing.Any]
	[typing.Callable[_P, _T]], typing.Callable[_P, _T]

Uh oh!

Conversation

Viicos commented Jun 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Uh oh!

Uh oh!

adriangb left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adriangb commented Jun 10, 2023

Uh oh!

Viicos commented Jun 10, 2023

Uh oh!

adriangb commented Jun 10, 2023

Uh oh!

adriangb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Viicos Jun 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kludex commented Jun 20, 2023

Uh oh!

Viicos commented Jun 20, 2023

Uh oh!

Kludex commented Jun 20, 2023

Uh oh!

Viicos commented Jun 20, 2023

Uh oh!

Uh oh!

Kludex commented Jun 22, 2023

Uh oh!

Viicos commented Jun 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Viicos Jul 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kludex commented Jul 6, 2023

Uh oh!

Viicos commented Jul 6, 2023

Uh oh!

Kludex commented Jul 13, 2023

Uh oh!

Viicos commented Jul 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Viicos commented Jun 10, 2023 •

edited

Loading

adriangb left a comment •

edited

Loading

Viicos Jun 10, 2023 •

edited

Loading

Viicos commented Jun 23, 2023 •

edited

Loading

Viicos Jul 6, 2023 •

edited

Loading

Viicos commented Jul 13, 2023 •

edited

Loading