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

Fix type hints for log_config #1539

Merged
merged 3 commits into from Jun 24, 2022
Merged

Fix type hints for log_config #1539

merged 3 commits into from Jun 24, 2022

Conversation

wch
Copy link
Contributor

@wch wch commented Jun 24, 2022

This is a fix for the issue reported here: #1536

Previously, if external code called uvicorn.run() and was type-checked with pyright, it reported error: Type of "run" is partially unknown. This is because it wants the generic type dict to have parameters specifying the type, as in dict[str, Any].

I'm actually a bit surprised that mypy doesn't also report this as a problem, since all the examples I saw in the mypy docs provide a parameter for generics like list and dict.

Kludex
Kludex approved these changes Jun 24, 2022
Copy link
Sponsor Member

@Kludex Kludex left a comment

Thanks @wch ! 🎉

I think it's normal for mypy not to warn here, as dict == Dict[str, Any]... It's just that Pyright on strict mode (I think?) see it as a problem... Or maybe mypy with more strict setup see it as a problem as well? 🤔

@wch
Copy link
Contributor Author

wch commented Jun 24, 2022

Ah, I didn't know that dict is equivalent to Dict[str, Any].

I did find this flag for mypy:
https://mypy.readthedocs.io/en/stable/error_code_list2.html#check-that-type-arguments-exist-type-arg

If you use --disallow-any-generics, mypy requires that each generic type has values for each type argument. For example, the types list or dict would be rejected. You should instead use types like list[int] or dict[str, int]. Any omitted generic type arguments get implicit Any values. The type list is equivalent to list[Any], and so on.

I tried using mypy with that option:

$ scripts/check
+ venv/bin/black --check --diff --target-version=py37 uvicorn tests
All done! ✨ 🍰 ✨
65 files would be left unchanged.
+ venv/bin/flake8 uvicorn tests
+ venv/bin/mypy --show-error-codes --disallow-any-generics
uvicorn/config.py:132: error: Missing type parameters for generic type "PathLike"  [type-arg]
uvicorn/config.py:133: error: Missing type parameters for generic type "PathLike"  [type-arg]
uvicorn/config.py:137: error: Missing type parameters for generic type "PathLike"  [type-arg]
uvicorn/config.py:212: error: Missing type parameters for generic type "Callable"  [type-arg]
uvicorn/config.py:225: error: Missing type parameters for generic type "PathLike"  [type-arg]
uvicorn/config.py:250: error: Missing type parameters for generic type "PathLike"  [type-arg]
uvicorn/config.py:525: error: Missing type parameters for generic type "Callable"  [type-arg]
uvicorn/server.py:44: error: Missing type parameters for generic type "Task"  [type-arg]
uvicorn/server.py:87: error: Missing type parameters for generic type "list"  [type-arg]
tests/conftest.py:149: error: Missing type parameters for generic type "dict"  [type-arg]
uvicorn/workers.py:35: error: Missing type parameters for generic type "dict"  [type-arg]
tests/test_config.py:42: error: Missing type parameters for generic type "dict"  [type-arg]
tests/test_config.py:47: error: Missing type parameters for generic type "dict"  [type-arg]
tests/test_config.py:340: error: Missing type parameters for generic type "typing.Callable"  [type-arg]
tests/test_config.py:390: error: Missing type parameters for generic type "dict"  [type-arg]
tests/test_config.py:411: error: Missing type parameters for generic type "dict"  [type-arg]
Found 16 errors in 5 files (checked 53 source files)

@adriangb
Copy link
Sponsor

adriangb commented Jun 24, 2022

I think we should use Mapping[str, Any]: run should not mutate the dict it is passed in.

@Kludex
Copy link
Sponsor Member

Kludex commented Jun 24, 2022

I think we should use Mapping[str, Any]: run should not mutate the dict it is passed in.

The log_config does mutate internally.

uvicorn/uvicorn/config.py

Lines 399 to 407 in 0954e56

if self.log_config is not None:
if isinstance(self.log_config, dict):
if self.use_colors in (True, False):
self.log_config["formatters"]["default"][
"use_colors"
] = self.use_colors
self.log_config["formatters"]["access"][
"use_colors"
] = self.use_colors

uvicorn/main.py Outdated Show resolved Hide resolved
@adriangb
Copy link
Sponsor

adriangb commented Jun 24, 2022

The log_config does mutate internally.

It should make a copy then (maybe another PR)

@Kludex Kludex merged commit 837fd21 into encode:master Jun 24, 2022
12 checks passed
@wch wch deleted the fix-type branch Jun 24, 2022
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.

None yet

3 participants