Suggestion: set `NotRequired` on all fields that aren't required

[jakkdl]

Reading through the spec and asgiref/typing.py I noticed that NotRequired was used for a few fields, but far from all fields that aren't required. It seems that state got marked as NotRequired in #354 for backwards compatibility reasons, but I don't see why the marker shouldn't also be used for all fields that are mentioned as ""Optional"" in the spec.

This arose as I was working on hypercorn, which bundles a copy of the typing spec, where the equivalent of this failed to typecheck:

from asgiref.typing import HTTPRequestEvent

e: HTTPRequestEvent = {"type": "http.request"}

But it clearly ""should"", given that https://asgi.readthedocs.io/en/latest/specs/www.html#request-receive-event says content and more_body are optional keys. But the types doesn't reflect that, leading to an error

https://github.com/django/asgiref/blob/db3ff43e9fa1daf7c6b1cb67db8eec1885be911d/asgiref/typing.py#L110-L113

[Kludex]

I agree with this. The problem is that the spec is ambiguous because not required is interpreted as None sometimes.

I've completely redo the typing on the uvicorn.typing module.

[Kludex]

Also, I don't like the typing module lives on the asgiref package.

[jakkdl]

Yeah it's not great that asgi-types is separate and have deviated from the ones in this package. Looking at asgi-types it is much more liberal with NotRequired (https://github.com/Kludex/asgi-types/blob/main/asgi_types/__init__.py), but at least in my interpretation of the spec it is missing several ones, e.g. HTTPRequestEvent has more_body but not body marked.

And yeah the spec should probably avoid the term "Optional" for non-required fields when that usually means | None.

[andrewgodwin]

Generally, if the spec says a field is optional, the intent there is that its key can be missing (i.e. it is NotRequired). Happy to take a PR to fix these up.

[Kludex]

Generally, if the spec says a field is optional, the intent there is that its key can be missing (i.e. it is NotRequired). Happy to take a PR to fix these up.

There was actually a previous discussion about this when the typing module was introduced by Phil. FYI

[Kludex]

Yeah it's not great that asgi-types is separate and have deviated from the ones in this package. Looking at asgi-types it is much more liberal with NotRequired (Kludex/asgi-types@main/asgi_types/init.py), but at least in my interpretation of the spec it is missing several ones, e.g. HTTPRequestEvent has more_body but not body marked.

And yeah the spec should probably avoid the term "Optional" for non-required fields when that usually means | None.

This is more up-to-date: https://github.com/encode/uvicorn/blob/master/uvicorn/_types.py

[jakkdl]

Yeah it's not great that asgi-types is separate and have deviated from the ones in this package. Looking at asgi-types it is much more liberal with NotRequired (Kludex/asgi-types@main/asgi_types/init.py), but at least in my interpretation of the spec it is missing several ones, e.g. HTTPRequestEvent has more_body but not body marked. And yeah the spec should probably avoid the term "Optional" for non-required fields when that usually means | None.

This is more up-to-date: https://github.com/encode/uvicorn/blob/master/uvicorn/_types.py

sooo, any reason not to copy these upstream? Would be nice if projects like uvicorn or hypercorn could import the types from asgi-types and ensure those are kept up to date instead of keeping slightly different copies in their own repositories

[Kludex]

Time... @jakkdl I've invited you as a collaborator to asgi-types.