Invalid usage of optional parameter / missing usage of Optional types

[dctr]

A lot of parameters in httpx are provided a fallback-value of None, whilst the respective types are not indicating being optional.

Example from https://github.com/encode/httpx/blob/master/httpx/_client.py

    async def request(
        self,
        method: str,
        url: URLTypes,
        *,
        content: RequestContent = None,
        data: RequestData = None,
        files: RequestFiles = None,
        json: typing.Any = None,
        params: QueryParamTypes = None,
        headers: HeaderTypes = None,
        cookies: CookieTypes = None,
        auth: typing.Union[AuthTypes, UnsetType] = UNSET,
        allow_redirects: bool = True,
        timeout: typing.Union[TimeoutTypes, UnsetType] = UNSET,
    ) -> Response:
        # ...

with for example

RequestContent = Union[str, bytes, Iterable[bytes], AsyncIterable[bytes]]

It should either be

# parameter
content: Optional[RequestContent] = None
# or type
RequestContent = Union[str, bytes, Iterable[bytes], AsyncIterable[bytes], None]  # including None

The latter would align with what is done e.g. in https://github.com/encode/httpx/blob/master/httpx/_types.py with

AuthTypes = Union[
    Tuple[Union[str, bytes], Union[str, bytes]],
    Callable[["Request"], "Request"],
    "Auth",
    None,
]

From personal preference I'd be in favour of the former though, as it makes the type-signature more meaningful.

For context, I currently have a wrapper function around request, but have to silence type warnings.

[FasterSpeeding]

#1439 seems related to this but as random input i've also had a similar issue with having to silence type warnings when wrapping the AsyncClient

[tomchristie]

I mean it's a real mixed bag. Yeah we could but it'd add an awful lot of extra noise to the codebase.

I guess I'd be happy to review how the change looked if someone wanted to go ahead and issue a pull request that changed all those signatures, and switched up our own mypy rules to enforce it, but I certainly wouldn't be able to say if we'd accept the pull request or not, so it'd need someone who's willing to put in that time, without having any guarantees that it'd make it in.

[tomchristie]

Some good points there. How does this middle ground sound?...

If someone's willing to issue a pull request that just changes the signatures in client.py, as a starting point, then I'd be willing to take a call on if its worth pushing forward with switching the signatures throughout?

That gives us a visible change against which to judge the trade-offs.

[dctr]

Thanks for that @tomchristie , I'll find some time later this week and give that a shot, both with Optional[T] as well as Union[..., None] and create two PRs for that.

[dctr]

Hey @tomchristie , here's a draft PR having a stab at all the types in client.py using the Union[..., None] approach.

I chose to try this approach first, as it is already done for some other types and I thought it might have a higher chance of being accepted. However this approach has the clear shortcomming that one is not able to distinguish out when a value is not None anymore after a None-check. There is an example in the PR. So this will not solve the problem.

#1760

As pointed out above, Optional is the intended tool for this scenario and the issue that surfaced in the PR is a good example for that.

---

There are already some Optional[T]s in there, so you can see a bit of how it could look like. I would be up to giving it a shot in another PR if you would be open to consider it. Also, with directly import in types from typing, the code becomes much more concise.

[tomchristie]

Okay, in that case I think we should close off #1760, right?

There are already some Optional[T]s in there, so you can see a bit of how it could look like. I would be up to giving it a shot in another PR if you would be open to consider it. Also, with directly import in types from typing, the code becomes much more concise.

I think the sensible thing here would be to treat each part separately, so firstly just issuing a pull request which changes the import style of the typing module, without making any other changes.

[dctr]

Here we go: #1769

Looks much less noisy now, and I reckon making things Optional[T] will also look much cleaner after this.