Add test to check if run() matches Config.__init__()
#1545
Conversation
| config_params = { | ||
| key: repr(value) | ||
| for key, value in inspect.signature(Config.__init__).parameters.items() | ||
| if key not in ("self", "timeout_notify", "callback_notify") |
There was a problem hiding this comment.
timeout_notify and callback_notify are not available via run(). Should `run() support it?
There was a problem hiding this comment.
Probably good to make a constant with those two values CONFIG_ONLY_PARAMS
There was a problem hiding this comment.
timeout_notifyandcallback_notifyare not available viarun(). Should `run() support it?
certainly not imho callback_notify it is just there to let the gunicorn know the worker is still alive iirc every timeout_notify
those can be overridden if you are subclassing UvicornWorker, I wouldn't know how to do that with the CLI nor if it makes sense
Probably good to make a constant with those two values
CONFIG_ONLY_PARAMS
why not yes, not opposed having them that way too
There was a problem hiding this comment.
certainly not imho callback_notify it is just there to let the gunicorn know the worker is still alive iirc every timeout_notify
those can be overridden if you are subclassing UvicornWorker, I wouldn't know how to do that with the CLI nor if it makes sense
Right. Thanks 🙏
There was a problem hiding this comment.
I don't see much benefit on the CONFIG_ONLY_PARAMS constant for a single test function. Is there?
I'll merge this, but I don't mind creating a PR to add that. 🙏
| @@ -277,7 +277,7 @@ def __init__( | |||
| self.interface = interface | |||
| self.debug = debug | |||
| self.reload = reload | |||
| self.reload_delay = reload_delay or 0.25 | |||
| self.reload_delay = reload_delay | |||
There was a problem hiding this comment.
There's no need to accept None for this. The behavior is not changed with this change.
There was a problem hiding this comment.
I agree it's cleaner but what if someone explicitly called it with reload_delay=None? The default value won't override it. Might be worth a mention in the changelog
There was a problem hiding this comment.
AFAIC, we shouldn't check a behavior that it's not allowed by the input, considering the type hint. I mean, reload_delay annotation changed from Optional[float] to float on this PR.
But... As you've pointed out, this is a breaking change, and I don't want to add breaking changes on this PR. 😅
I'll revert this change, and adapt the run(). 🙏
There was a problem hiding this comment.
Ok. I thought about it... Yep, you're right. I'll follow the path you suggested and add a note on the changelog about this.
| config_params = { | ||
| key: repr(value) | ||
| for key, value in inspect.signature(Config.__init__).parameters.items() | ||
| if key not in ("self", "timeout_notify", "callback_notify") |
There was a problem hiding this comment.
timeout_notifyandcallback_notifyare not available viarun(). Should `run() support it?
certainly not imho callback_notify it is just there to let the gunicorn know the worker is still alive iirc every timeout_notify
those can be overridden if you are subclassing UvicornWorker, I wouldn't know how to do that with the CLI nor if it makes sense
Probably good to make a constant with those two values
CONFIG_ONLY_PARAMS
why not yes, not opposed having them that way too
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.
After breaking the logs on 0.18.0 (see the fix on #1541) because of #1423 I've been thinking of a way to have avoided that...
I came up with this. This PR adds a test that matches the type annotation and default values of
run()andConfig.__init__().This PR also fixes the mismatches.