Switch to explicit typing.Optional throughout

[michaeloliverx]

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]

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]

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

[tomchristie]

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]

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]

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]

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]

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]

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

[michaeloliverx]

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

[michaeloliverx]

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]

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]

@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 <michaeloliver__@outlook.com>
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]

@vfazio thanks!

[adriangb]

This looks ready to me!

[tomchristie]

Sure thing, let's do it.