Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add typing for ASGI Scopes, messages, and frameworks #221

Merged
merged 2 commits into from
Jan 17, 2021

Conversation

pgjones
Copy link
Contributor

@pgjones pgjones commented Dec 24, 2020

These are useful for users of asgiref to type check their ASGI
usage. It also serves as the reference typing for the ASGI
Specification.

@pgjones
Copy link
Contributor Author

pgjones commented Dec 24, 2020

These are from Hypercorn (so I think they are correct, or Hypercorn is wrong :o).

@pgjones pgjones force-pushed the typing branch 2 times, most recently from 3c0f46c to 29d7c1a Compare December 24, 2020 18:19
@euri10
Copy link
Contributor

euri10 commented Dec 24, 2020 via email

@andrewgodwin
Copy link
Member

You are a wonderful person for doing this, I was just thinking about adding typing now we're using it more at work. Get that linter happy and I will smash the merge button.

@andrewgodwin
Copy link
Member

(and maybe start working these into the actual ASGI code over the xmas break, since it's about type we had a little typing in there alongside tests)

Copy link
Contributor

@florimondmanca florimondmanca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super nice :)

We'll be able to properly use these in eg Uvicorn, Starlette or arbitrary apps / frameworks by adding a dependency on asgiref then, correct?

I’ll need to update awesome-asgi to mention that asgiref now ships reference type hints as well 😃

asgiref/typing.py Outdated Show resolved Hide resolved
@florimondmanca
Copy link
Contributor

florimondmanca commented Dec 24, 2020

@pgjones Do we need to add a py.typed containing partial\n? I believe that's required for mypy to parse through asgiref when importing it from other packages — or we'd get "Module asgiref is missing" errors or similar. Might also require a tweak to include_package_data in setup.py? I guess we'd have to check on a dummy test package that type hints would be correctly recognized and used by mypy.

@pgjones
Copy link
Contributor Author

pgjones commented Dec 24, 2020

asgiref itself isn't typed, so a py.typed at this stage would be misleading (although yes when it is :)). Mypy will be able to understand these typings if they are imported and used.

asgiref/typing.py Outdated Show resolved Hide resolved
@tiangolo
Copy link
Contributor

This is awesome! 🚀

A small note to have in mind, as @Kludex points out, there's probably a caveat with the optionals.

I don't see an obvious way to solve it, as I don't see a standard way to declare these optional types perfectly, so I'll just share the info I have (as I understand it).


The issue is that Optional[SomeType] actually means "the union of SomeType and None". But that would imply an explicit None, not that the key could be entirely omitted from the dictionary.

And then, using TypedDict, it is assumed that all the keys are required: https://docs.python.org/3/library/typing.html#typing.TypedDict

By default, all keys must be present in a TypedDict. It is possible to override this by specifying totality.

And the example is:

class point2D(TypedDict, total=False):
    x: int
    y: int

But then that means that any of the keys can be omitted, which is also not what is wanted here.

My point is, there is no official way to type a dictionary that could have some specific keys missing. I think the only option here is to assume a convention and take the drawbacks:

  1. Declare the dicts using total=False, although that makes all the keys possibly missing, which is not great.
  2. Mark the optional keys with Optional[X], although that will really mean Union[X, None], and when None is not passed explicitly in the dict, linters will (or should/would) complain, which is also not great.

I would personally prefer option 2, as that would be annoying when writing code showing false-positives but at least wouldn't accept invalid code that would later break in production. But that's also in my subjective point of view.


In any case, I think these types are awesome, thanks for putting the effort to write them. 🤓 🚀 ☕

@florimondmanca
Copy link
Contributor

florimondmanca commented Dec 26, 2020

There's a 3rd option, which is duplicating the dict, with one version containing the key and the other not, and declaring a Union of those as the final type. Convoluted and unsuitable if there are many non mandatory keys. But maybe... ? Edit: hmm, maybe not. 😜

Otherwise agreed with @tiangolo, option 2 (which is almost the current state?) sounds like the most suitable!

@tiangolo
Copy link
Contributor

Ah, very clever @florimondmanca ! 🤓 🚀

I think that would be the more formal way to do it indeed. I guess it would get problematic to maintain here in the spec for dicts with several optional parameters, as it would require a lot of types with each of the possible combinations (I think "combinatorial explosion" is the term 🤔). But yeah, maybe that could work for some cases? I don't know.

@pgjones
Copy link
Contributor Author

pgjones commented Dec 26, 2020

I thought about (how to type) omitted keys for Hypercorn, and my view is that there isn't a worthwhile advantage to omitting a key. This is actually the area in which I've had the most ASGI-bugs with Quart/Hypercorn and I'd prefer the specification to state that the keys must be present with a default value.

Practically, using these type hints will result in mypy insisting that the defaults be added when scopes/messages are created, whilst not affecting using messages/scopes. I think this is an acceptable trade off given that trying to type the optionality of the current ASGI spec is quite tricky using the current tooling.

Copy link
Contributor

@florimondmanca florimondmanca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stellar. 😄

These are useful for users of asgiref to type check their ASGI
usage. It also serves as the reference typing for the ASGI
Specification.
This allows isort and black to play nicely for a long import line in
the typing module.
Copy link
Contributor

@tiangolo tiangolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks awesome! 👏 🚀

I just checked all the code while re-reading the spec part by part. Great job! 🤓 ☕

@sm-Fifteen
Copy link

sm-Fifteen commented Jan 14, 2021

Using unions instead of a class hierarchy is also useful to avoid running into python/mypy#7435, since TypedDict also does not support changing the type of a field in a subclass, regardless of compatibility between field types.

There's a 3rd option, which is duplicating the dict, with one version containing the key and the other not, and declaring a Union of those as the final type. Convoluted and unsuitable if there are many non mandatory keys. But maybe... ? Edit: hmm, maybe not. 😜

Otherwise agreed with @tiangolo, option 2 (which is almost the current state?) sounds like the most suitable!

@florimondmanca: How about splitting the type to have a total dict with only the required keys, a partial dict with only the optional keys, and declaring the real type as inheriting both?

from typing import Awaitable, Callable, Dict, Iterable, Optional, Tuple, Type, Union
from typing import Literal, Protocol, TypedDict

class HTTPRequestEventReq(TypedDict):
	type: Literal["http.request"]
	body: bytes

class HTTPRequestBodyOpt(TypedDict, total=False):
	more_body: bool

class HTTPRequestBody(HTTPRequestEventReq, HTTPRequestBodyOpt):
	pass

good_some: HTTPRequestBody = {
	'type': "http.request",
	'body': b'',
	#'more_body': False,
}

good_all: HTTPRequestBody = {
	'type': "http.request",
	'body': b'',
	'more_body': False,
}

bad_missing: HTTPRequestBody = {
	'type': "http.request",
	#'body': b'',
	'more_body': False,
}

bad_extra: HTTPRequestBody = {
	'type': "http.request",
	'body': b'',
	#'more_body': False,
	'foo': 'bar',
}

This gives the expected result (first two dicts are good, the other two raise errors) when I try running mypy on this file.

That doesn't address pgjones' concerns about bugs related to optional keys, but it allows to faithfully represent the spec and validate use of is on the sender's side (the receiver's side can still attempt to read an optional key without mypy raising any objections about the key being possibly missing).

@andrewgodwin
Copy link
Member

I'm happy with this as is; let me know if you want to merge it or if you want to refine the typing a little with the omitted keys handled somehow. I personally think it's OK to not worry about handling omissions and be complete, or we could bring in something else that handles them OK (I know Pydantic does this, but it's not quite standard typing)

@pgjones
Copy link
Contributor Author

pgjones commented Jan 16, 2021

I'd prefer to merge as is.

@andrewgodwin andrewgodwin merged commit cf99cb7 into django:master Jan 17, 2021
@andrewgodwin
Copy link
Member

Done! Thanks for your work on this, I'm excited to get the actual methods/functions typed now as well.

@graingert
Copy link
Contributor

Mypy will be able to understand these typings if they are imported and used.

mypy will ignore these types unless py.typed is present

@pgjones
Copy link
Contributor Author

pgjones commented Jan 23, 2021

mypy will ignore these types unless py.typed is present

I checked this again, and I was wrong mypy completely ignores without the marker (even when directly imported as I thought worked). See #231

@pgjones
Copy link
Contributor Author

pgjones commented Mar 22, 2021

PEP 655 is something we should keep an eye on - it will allow for required (and not required) typed dict keys if accepted.

@graingert
Copy link
Contributor

@pgjones it might be better to use @attr.s or @dataclasses.dataclass objects so defaults can be set cleanly

@sm-Fifteen
Copy link

PEP 655 is something we should keep an eye on - it will allow for required (and not required) typed dict keys if accepted.

@pgjones: Like the PEP highlights and I pointed out earlier, you can use inheritance between a total and a non-total typed dict to have both required and non-required keys. It's somewhat cumbersome, but it does work under current versions of Python.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants