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

Annotate run function #1423

Merged
merged 8 commits into from
May 10, 2022
Merged

Annotate run function #1423

merged 8 commits into from
May 10, 2022

Conversation

Kludex
Copy link
Member

@Kludex Kludex commented Mar 28, 2022

Alternative to #1421.

How does it look? 🤔

@euri10 euri10 mentioned this pull request Mar 28, 2022
@Kludex Kludex changed the title Annotate run function (2) Annotate run function Mar 28, 2022
@Kludex Kludex requested a review from PrettyWood April 7, 2022 11:23
@Kludex
Copy link
Member Author

Kludex commented Apr 7, 2022

@PrettyWood can we do better?

@Kludex Kludex added this to the Version 0.18.0 milestone Apr 23, 2022
@Kludex Kludex requested a review from adriangb April 26, 2022 16:24
@adriangb
Copy link
Member

I didn't test this with MyPy or anything, but I don't see any issues just looking at the code. As long as tests pass and you checked it against MyPy and Pylance in strict mode, I'm happy with this @Kludex

@Kludex Kludex requested a review from tomchristie May 3, 2022 11:57
@Kludex Kludex merged commit 208ef4e into master May 10, 2022
@Kludex Kludex deleted the annotate/run-alternative branch May 10, 2022 17:52
Kludex added a commit to sephioh/uvicorn that referenced this pull request Oct 29, 2022
* Annotate `run` function

* Fix mypy issue

* Annotate `run` function (2)

* Remove docstring

* Fix log_config type

* Ignore headers type
Kludex added a commit that referenced this pull request Oct 29, 2022
* Annotate `run` function

* Fix mypy issue

* Annotate `run` function (2)

* Remove docstring

* Fix log_config type

* Ignore headers type
br3ndonland added a commit to br3ndonland/inboard that referenced this pull request Dec 6, 2022
This commit will enable mypy strict mode, and update code accordingly.

Type annotations are not used at runtime. The standard library `typing`
module includes a `TYPE_CHECKING` constant that is `False` at runtime,
but `True` when conducting static type checking prior to runtime. Type
imports will be included under `if TYPE_CHECKING:` conditions. These
conditions will be ignored when calculating test coverage.
https://docs.python.org/3/library/typing.html

The Python standard library `logging.config` module uses type stubs.
The typeshed types for the `logging.config` module are used solely for
type-checking usage of the `logging.config` module itself. They cannot
be imported and used to type annotate other modules. For this reason,
dict config types will be vendored into a module in the inboard package.
https://github.com/python/typeshed/blob/main/stdlib/logging/config.pyi

The ASGI application in `inboard.app.main_base` will be updated to ASGI3
and type-annotated with `asgiref.typing`. Note that, while Uvicorn uses
`asgiref.typing`, Starlette does not. The type signature expected by the
Starlette/FastAPI `TestClient` therefore does not match
`asgiref.typing.ASGIApplication`. A mypy `type: ignore[arg-type]`
comment will be used to resolve this difference.
https://asgi.readthedocs.io/en/stable/specs/main.html

Also note that, while the `asgiref` package was a runtime dependency of
Uvicorn 0.17.6, it was later removed from Uvicorn's runtime dependencies
in 0.18.0 (encode/uvicorn#1305, encode/uvicorn#1532). However, `asgiref`
is still used to type-annotate Uvicorn, so any downstream projects like
inboard that type-check Uvicorn objects must also install `asgiref`.
Therefore, `asgiref` will be added to inboard's development dependencies
to ensure that type checking continues to work as expected.

A Uvicorn options type will be added to a new inboard types module.
The Uvicorn options type will be a `TypedDict` with fields corresponding
to arguments to `uvicorn.run`. This type can be used to check arguments
passed to `uvicorn.run`, which is how `inboard.start` runs Uvicorn.

Uvicorn 0.17.6 is not fully type-annotated, and Uvicorn does not ship
with a `py.typed` marker file until 0.19.0.

It would be convenient to generate types dynamically with something like
`getattr(uvicorn.run, "__annotations__")` (Python 3.9 or earlier)
or `inspect.get_annotations(uvicorn.run)` (Python 3.10 or later).
https://docs.python.org/3/howto/annotations.html

It could look something like this:

```py
UvicornOptions = TypedDict(  # type: ignore[misc]
    "UvicornOptions",
    inspect.get_annotations(uvicorn.run),
    total=False,
)
```

Note the `type: ignore[misc]` comment. Mypy raises a `misc` error:
`TypedDict() expects a dictionary literal as the second argument`.
Unfortunately, `TypedDict` types are not intended to be generated
dynamically, because they exist for the benefit of static type checking
(python/mypy#3932, python/mypy#4128, python/mypy#13940).

Furthermore, prior to Uvicorn 0.18.0, `uvicorn.run()` didn't enumerate
keyword arguments, but instead accepted `kwargs` and passed them to
`uvicorn.Config.__init__()` (encode/uvicorn#1423). The annotations from
`uvicorn.Config.__init__()` would need to be used instead. Even after
Uvicorn 0.18.0, the signatures of the two functions are not exactly the
same (encode/uvicorn#1545), so it helps to have a static type defined.

There will be some other differences from `uvicorn.run()`:

- The `app` argument to `uvicorn.run()` accepts an un-parametrized
  `Callable` because Uvicorn tests use callables (encode/uvicorn#1067).
  It is not necessary for other packages to accept `Callable`, and it
  would need to be parametrized to pass mypy strict mode anyway.
  For these reasons, `Callable` will not be accepted in this type.
- The `log_config` argument will use the new inboard dict config type
  instead of `dict[str, Any]` for stricter type checking.
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.

3 participants