Use mypy `strict` #2180

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? 🤔

starlette/pyproject.toml

Lines 68 to 71 in 1be57fd

 [[tool.mypy.overrides]] 

 module = "tests.*" 

 disallow_untyped_defs = false 

 check_untyped_defs = true

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

@@ -23,10 +29,14 @@ def requires(
 scopes: typing.Union[str, typing.Sequence[str]],
 status_code: int = 403,
 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

@@ -773,7 +783,7 @@ def host(
 def add_route(
 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:

starlette/starlette/routing.py

Lines 209 to 234 in 2168e47

def __init__(

self,

path: str,

endpoint: typing.Callable,

*,

methods: typing.Optional[typing.List[str]] = None,

name: typing.Optional[str] = None,

include_in_schema: bool = True,

) -> None:

assert path.startswith("/"), "Routed paths must start with '/'"

self.path = path

self.endpoint = endpoint

self.name = get_name(endpoint) if name is None else name

self.include_in_schema = include_in_schema

endpoint_handler = endpoint

while isinstance(endpoint_handler, functools.partial):

endpoint_handler = endpoint_handler.func

if inspect.isfunction(endpoint_handler) or inspect.ismethod(endpoint_handler):

# Endpoint is function or method. Treat it as `func(request) -> response`.

self.app = request_response(endpoint)

if methods is None:

methods = ["GET"]

else:

# Endpoint is a class. Treat it as ASGI.

self.app = endpoint

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

@@ -209,7 +209,7 @@ class Route(BaseRoute):
 def __init__(
 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 requested 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 Outdated 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 encode:master Jul 23, 2023
5 checks passed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use mypy `strict` #2180

Use mypy `strict` #2180

Viicos commented Jun 10, 2023 •

edited

Loading

adriangb left a comment •

edited

Loading

adriangb commented Jun 10, 2023

Viicos commented Jun 10, 2023

adriangb commented Jun 10, 2023

adriangb left a comment

Viicos Jun 10, 2023

Viicos Jun 10, 2023 •

edited

Loading

Kludex commented Jun 20, 2023

Viicos commented Jun 20, 2023

Kludex commented Jun 20, 2023

Viicos commented Jun 20, 2023

Kludex commented Jun 22, 2023

Viicos commented Jun 23, 2023 •

edited

Loading

Kludex Jun 30, 2023

Viicos Jul 6, 2023

Kludex Jun 30, 2023

Viicos Jul 1, 2023

Kludex Jun 30, 2023

Viicos Jul 6, 2023 •

edited

Loading

Kludex Jun 30, 2023

Kludex Jun 30, 2023

Kludex Jun 30, 2023

Viicos Jul 1, 2023

Kludex commented Jul 6, 2023

Viicos commented Jul 6, 2023

Kludex commented Jul 13, 2023

Viicos commented Jul 13, 2023 •

edited

Loading

Viicos commented Jul 16, 2023

Kludex left a comment

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

	def __init__(
	self,
	path: str,
	endpoint: typing.Callable,
	*,
	methods: typing.Optional[typing.List[str]] = None,
	name: typing.Optional[str] = None,
	include_in_schema: bool = True,
	) -> None:
	assert path.startswith("/"), "Routed paths must start with '/'"
	self.path = path
	self.endpoint = endpoint
	self.name = get_name(endpoint) if name is None else name
	self.include_in_schema = include_in_schema

	endpoint_handler = endpoint
	while isinstance(endpoint_handler, functools.partial):
	endpoint_handler = endpoint_handler.func
	if inspect.isfunction(endpoint_handler) or inspect.ismethod(endpoint_handler):
	# Endpoint is function or method. Treat it as `func(request) -> response`.
	self.app = request_response(endpoint)
	if methods is None:
	methods = ["GET"]
	else:
	# Endpoint is a class. Treat it as ASGI.
	self.app = endpoint

Use mypy strict #2180

Use mypy strict #2180

Conversation

Viicos commented Jun 10, 2023 • edited Loading

Checklist

adriangb left a comment • edited Loading

Choose a reason for hiding this comment

adriangb commented Jun 10, 2023

Viicos commented Jun 10, 2023

adriangb commented Jun 10, 2023

adriangb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Viicos Jun 10, 2023 • edited Loading

Choose a reason for hiding this comment

Kludex commented Jun 20, 2023

Viicos commented Jun 20, 2023

Kludex commented Jun 20, 2023

Viicos commented Jun 20, 2023

Kludex commented Jun 22, 2023

Viicos commented Jun 23, 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

Viicos Jul 6, 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

Kludex commented Jul 6, 2023

Viicos commented Jul 6, 2023

Kludex commented Jul 13, 2023

Viicos commented Jul 13, 2023 • edited Loading

Viicos commented Jul 16, 2023

Kludex left a comment

Choose a reason for hiding this comment

Use mypy `strict` #2180

Use mypy `strict` #2180

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