[adriangb]

#2404 (comment)

Removing TypeError checks. Personally I'm probably against this, but more importantly let's discuss that as a separate issue.

My rationale for removing this: we should either adopt runtime type checking systematically (using beartype, Pydantic's validation decorator or making our inputs (like a Request instance) Pydantic models) or we should remove it all together and rely on static type checking.

I think having manual runtime type checking sprinkled around the codebase doesn't scale as well as either of those two solutions.

Amongst those two I think the obvious choice is to rely purely on static type checking and documentation. It's more valuable for users to know upfront that they're giving us the wrong type (e.g. by implementing #2404 which replaces an Any for the value with the types that we are checking for at runtime here) than to do it at runtime.

[lovelydinosaur]

Hrm... I'm not convinced about this. We do get feedback where the TypeError checking is giving more useful errors then we'd otherwise be seeing. For example the answer to this #2402 Although, yes, I can see the argument for rationalising our TypeErrors.

A good question for giving us some guidance onto this would be looking at the top-level httpx.request(...) function, and reviewing which arguments are and are not strictly checked right now?

[lovelydinosaur]

Oh, and yes, beartype and the pydantic decorator are both really neat - I'd not seen those before.

I'd prefer "really comprehensive runtime checking" over "type hints only". Just seems a much better UX.
Although I also really want to avoid adding dependancies, so.

[lovelydinosaur]

@adriangb - How would you like to proceed with this?

[adriangb]

I’d personally like to move forward with this but if you still feel like it’s better to keep runtime type checking around feel free to close it.

[adriangb]

Another option @tomchristie: we make something like Pydantic or beartype an optional dependency gated by an extra. We can then have a decorator that does nothing at runtime if that extra isn't activated or does runtime type checking if it is. That way if users show up with an issue that seems like it may be related to them passing in the wrong type we can say "please add the httpx[typecheck] extra and run again" as a debugging step.

[michaeloliverx]

I don't think its worth adding additional dependencies even if they were optional. I would be in favour of just removing the checks altogether and relying on static type checking.

[lovelydinosaur]

I don't think its worth adding additional dependencies even if they were optional.

With regards to type checking, I agree.

I would be in favour of just removing the checks altogether and relying on static type checking.

Are we referring to specifically just the checks in this pull request, or accepting the issue more generally?

[michaeloliverx]

I would be in favour of just removing the checks altogether and relying on static type checking.

Are we referring to specifically just the checks in this pull request, or accepting the issue more generally?

I was referring to specifically just the checks in this pull request. I am not sure on the broader sense. Most of the code I personally write is internal in an environment where we know we are using static analysis to catch potential type errors. Library code is somewhat different as we can't really know if users are using tools like mypy/pyright so I guess we should be doing some checks at the boundaries where users primarily interact?

I am fine with isinstance checks sprinkled where needed and would only reach for a tool like pydantic when it becomes a hindrance such as verifying large JSON bodies.

[adriangb]

Most of the code I personally write is internal in an environment where we know we are using static analysis to catch potential type errors. Library code is somewhat different as we can't really know if users are using tools like mypy/pyright so I guess we should be doing some checks at the boundaries where users primarily interact?

I agree with this, I'm in the same boat. I'm a bit torn because for me, personally, the runtime type checking is only hurting performance and making the code harder to iterate on. I don't run into these sorts of issues because type checkers and IDEs prevent them before any code is even run. But you made a great case for keeping the runtime type checking @tomchristie by pointing to frequent issues/discussions where it's helped users. To the point where I'm not even sure this change is a good one. In fact, the most recent one in #2432 is actually a case where there's an argument to be made for more runtime type checking. But ad-hoc testing is always going to have gaps, especially with all of our arguments that accept unions and whatnot. I kind of want to just make Request a Pydantic model because it would be easy for us maintainers and result in full validation of users inputs with really nice errors if they get something wrong, but I also think that adding it as a dependency might be a bit heavy and I could see other users of static type checkers being upset. Hmm.

[lovelydinosaur]

I'm a bit torn because for me, personally, the runtime type checking is only hurting performance and making the code harder to iterate on.

My suggestion would be closing this and #2404, and that we start with an issue similar to #2436 which digs into...

• Which models do we need to think about type checking for.
• Enumerating what type arguments we currently have, and which of those do/do not have runtime checks.

That'll at least give us a point of reference from which to work, so that whichever resolution we choose to go with, we can make sure we've got a more consistent approach throughout.

Make sense?

[tiangolo]

Just a quick couple of thoughts to add more confusion 😅

Pydantic v2 is around the corner. It will have types in general as a first class citizen, not only models (classes). That might help simplifying using it in some way (if at all).

I guess it could be an optional dependency that is only used when installed, but that always means a bit extra complexity in the codebase importing things and if they are not available adding dummy replacements. But maybe it could be done.

Another idea to maybe consider (although I'm not even checking the code right now as I'm on my phone), an option is to make some classes dataclasses, and optionally importing Pydantic's dataclasses when available, otherwise standard dataclasses.

Anyway, just a couple of thoughts, maybe too random but worth sharing just in case. 🤓🤷

[adriangb]

A thought here: could we feature flag runtime type checking behind an env var? That way if a user reports a problem that seems like it might be caused by their erroneous input we can say "please run your code with HTTPX_RUNTIME_TYPE_CHECKS=1" or something like that while the rest of the users won't be impacted. This feature flag could do something like:

# inside some utils.py type file
def maybe_runtime_type_check(func: T) -> T:
    if os.environ.get("HTTPX_RUNTIME_TYPE_CHECKS") is not None:
        try:
            from pydantic import validate_arguments
        except ImportError as e:
            raise RuntimeError("Runtime type checking requires ...") from e
        return validate_arguments(func)
    return func

# our main request class
class Request:
    @maybe_runtime_type_check
    def __init__(self, ...):  ...

The reason I made it check for the env var every time is so that it can be set dynamically in tests, to debug just one endpoint, etc. with minimal impact on other code execution.

[lovelydinosaur]

Similar runtime-type-checking-is-good-ux cases #2491 and #2339.

My project manager hat would suggest that we close this PR off, and that if we want to consider different approaches to runtime type checking then we start that off as a discussion.

[adriangb]

That sounds reasonable to me