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

Switch to explicit typing.Optional throughout #2096

Merged
merged 14 commits into from
May 20, 2022

Conversation

michaeloliverx
Copy link
Member

Fixes #2092

from contextlib import AsyncExitStack
import httpx


async def main() -> None:
    async with AsyncExitStack() as stack:
        async_http_client = await stack.enter_async_context(httpx.AsyncClient())
        reveal_type(async_http_client)

        sync_http_client = stack.enter_context(httpx.Client())
        reveal_type(sync_http_client)
❯ mypy test-script.py --strict
test-script.py:8: note: Revealed type is "httpx._client.AsyncClient*"
test-script.py:11: note: Revealed type is "httpx._client.Client*"

@florimondmanca
Copy link
Member

florimondmanca commented Feb 28, 2022

Thanks @michaeloliverx

This seems right to me.

But it also looks like this would belong to a wider "do we want to comply with --strict mypy" discussion, right? Right now we have no way to prevent a regression on this on CI, because we only run mypy (no strict mode).

Edit: see #1747

@florimondmanca
Copy link
Member

Happy to read @tomchristie's viewpoint on this -- should we start a discussion on mypy --strict, go ahead with this change, something else?

@tomchristie
Copy link
Member

Thanks @michaeloliverx, @florimondmanca.

Happy to read @tomchristie's viewpoint on this -- should we start a discussion on mypy --strict, go ahead with this change, something else?

Interesting, right? It's a little bit niggly that typing in Python ends up having this effect. We've got correct and consistent typing throughout, that helps us have confidence in changes within our codebase. But if our users choose to run mypy --strict, then we end up having to conform to that too.

I'd suggest that we value consistency above one-off fixes. So... we ought to consider a PR that switches to explicit typing.Optional everywhere, rather in just one place.

Right now we have no way to prevent a regression on this on CI, because we only run mypy (no strict mode).

This is a good point. A smart first pass at this would be a pull request that just turns on strict mode for our type checking. That'll fail, but it'd give us the complete set of changes that we'd need to make if we wanted to change this.

@michaeloliverx
Copy link
Member Author

michaeloliverx commented Feb 28, 2022

Currently --strict enables the following flags:

  --warn-unused-configs
  --disallow-any-generics
  --disallow-subclassing-any
  --disallow-untyped-calls
  --disallow-untyped-defs
  --disallow-incomplete-defs
  --check-untyped-defs
  --disallow-untyped-decorators
  --no-implicit-optional
  --warn-redundant-casts
  --warn-unused-ignores
  --warn-return-any
  --no-implicit-reexport
  --strict-equality

Enabling this may generate a lot of noise unrelated to using explicit typing.Optional expressions.

This is a good point. A smart first pass at this would be a pull request that just turns on strict mode for our type checking. That'll fail, but it'd give us the complete set of changes that we'd need to make if we wanted to change this.

We could enable --no-implicit-optional rather than --strict which would result in less changes and still achieve the desired effect of using explicit typing.Optional.

@michaeloliverx michaeloliverx changed the title Fix issue with Mypy --strict and AsyncExitStack Switch to explicit typing.Optional throughout Feb 28, 2022
@michaeloliverx
Copy link
Member Author

Okay I have now enabled no_implicit_optional in the mypy config file and fixed all the issues in httpx. I am still getting some errors:

httpx/_transports/default.py:173: error: Argument "proxy_auth" to "SOCKSProxy" has incompatible type "Optional[Tuple[bytes, bytes]]"; expected "Tuple[Union[bytes, str], Union[bytes, str]]"
httpx/_transports/default.py:176: error: Argument "max_keepalive_connections" to "SOCKSProxy" has incompatible type "Optional[int]"; expected "int"
httpx/_transports/default.py:177: error: Argument "keepalive_expiry" to "SOCKSProxy" has incompatible type "Optional[float]"; expected "float"
httpx/_transports/default.py:267: error: Argument "max_keepalive_connections" to "AsyncConnectionPool" has incompatible type "Optional[int]"; expected "int"
httpx/_transports/default.py:268: error: Argument "keepalive_expiry" to "AsyncConnectionPool" has incompatible type "Optional[float]"; expected "float"
httpx/_transports/default.py:271: error: Argument "uds" to "AsyncConnectionPool" has incompatible type "Optional[str]"; expected "str"
httpx/_transports/default.py:272: error: Argument "local_address" to "AsyncConnectionPool" has incompatible type "Optional[str]"; expected "str"
httpx/_transports/default.py:283: error: Argument "proxy_auth" to "AsyncHTTPProxy" has incompatible type "Optional[Tuple[bytes, bytes]]"; expected "Tuple[Union[bytes, str], Union[bytes, str]]"
httpx/_transports/default.py:287: error: Argument "max_keepalive_connections" to "AsyncHTTPProxy" has incompatible type "Optional[int]"; expected "int"
httpx/_transports/default.py:288: error: Argument "keepalive_expiry" to "AsyncHTTPProxy" has incompatible type "Optional[float]"; expected "float"
httpx/_transports/default.py:308: error: Argument "proxy_auth" to "AsyncSOCKSProxy" has incompatible type "Optional[Tuple[bytes, bytes]]"; expected "Tuple[Union[bytes, str], Union[bytes, str]]"
httpx/_transports/default.py:311: error: Argument "max_keepalive_connections" to "AsyncSOCKSProxy" has incompatible type "Optional[int]"; expected "int"
httpx/_transports/default.py:312: error: Argument "keepalive_expiry" to "AsyncSOCKSProxy" has incompatible type "Optional[float]"; expected "float"
httpx/_transports/default.py:332: error: Argument 1 to "__aexit__" of "AsyncConnectionPool" has incompatible type "Optional[Type[BaseException]]"; expected "Type[BaseException]"
httpx/_transports/default.py:332: error: Argument 2 to "__aexit__" of "AsyncConnectionPool" has incompatible type "Optional[BaseException]"; expected "BaseException"
httpx/_transports/default.py:332: error: Argument 3 to "__aexit__" of "AsyncConnectionPool" has incompatible type "Optional[TracebackType]"; expected "TracebackType"
Found 16 errors in 1 file (checked 60 source files)

I think these issues are actually with httpcore and the types passed to them are not compatible now.

@adriangb
Copy link
Member

adriangb commented Mar 1, 2022

Those some like mostly internal issues, right? Can we just #type: ignore[arg-type] them so that we can move forward with the typing changes in the public API?

@michaeloliverx
Copy link
Member Author

Those some like mostly internal issues, right? Can we just #type: ignore[arg-type] them so that we can move forward with the typing changes in the public API?

Yes we could do that for now.

@michaeloliverx
Copy link
Member Author

Okay this is ready now, I have ignored internal type errors for now.

@michaeloliverx
Copy link
Member Author

I opened a similar PR at encode/httpcore#513. If that is approved then we could remove the type ignore comments.

mnixry added a commit to mnixry/nonebot-plugin-gocqhttp that referenced this pull request Mar 4, 2022
@michaeloliverx
Copy link
Member Author

Now that encode/httpcore#513 has been merged I think we should wait until httpcore cuts a release before merging this as then we can remove all of the type: ingore comments.

@vfazio
Copy link
Contributor

vfazio commented May 17, 2022

Now that encode/httpcore#513 has been merged I think we should wait until httpcore cuts a release before merging this as then we can remove all of the type: ingore comments.

@michaeloliverx
0.15.0 was released a few hours ago.

@vfazio
Copy link
Contributor

vfazio commented May 19, 2022

@michaeloliverx

running your branch and updating httpcore:

(venv) vfazio@vfazio2 /tmp/tmp.OcS7trgNiz $ git diff
diff --git a/setup.py b/setup.py
index 0ca769f..f6edfdd 100644
--- a/setup.py
+++ b/setup.py
@@ -60,7 +60,7 @@ setup(
         "charset_normalizer",
         "sniffio",
         "rfc3986[idna2008]>=1.3,<2",
-        "httpcore>=0.14.5,<0.15.0",
+        "httpcore>=0.15.0,<0.16.0",
     ],
     extras_require={
         "http2": "h2>=3,<5",
(venv) vfazio@vfazio2 /tmp/tmp.OcS7trgNiz $ git log -n1
commit 1dc160b8a325219047425ac4914e1d10106dbee9 (HEAD -> michaeloliverx/issue2092, origin/michaeloliverx/issue2092)
Merge: 7caf37b 9673a35
Author: Michael Oliver <[email protected]>
Date:   Wed May 18 09:49:45 2022 +0100

    Merge branch 'master' into michaeloliverx/issue2092

(venv) vfazio@vfazio2 /tmp/tmp.OcS7trgNiz :( $ mypy httpx tests
Success: no issues found in 60 source files

@michaeloliverx
Copy link
Member Author

@vfazio thanks!

@adriangb
Copy link
Member

This looks ready to me!

@tomchristie
Copy link
Member

Sure thing, let's do it.

@tomchristie tomchristie merged commit 14a1704 into encode:master May 20, 2022
@michaeloliverx michaeloliverx deleted the michaeloliverx/issue2092 branch May 20, 2022 10:30
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.

Switch to explicit typing.Optional throughout
5 participants