Httpx: On type annotation style.

[tomchristie]

In large portions of our Client code we're creating type shorthands like TimeoutTypes and then using those everywhere, rather than listing a more verbose form, such as Union[None, int, Sequence[int], Timeout].

Although that gives us a more concise style I think it's also got some disadvantages...

It makes it harder to figure out what the type annotation actually is, and it leads to a bunch of type classes that are not public API, but which users might end up trying to treat as such.

I think it might well be worth us exploring what the codebase would look like if we tries to use more explicit annotation styles throughout.

class BaseClient:
    def __init__(
        self,
        *,
        auth: AuthTypes = None,
        params: QueryParamTypes = None,
        headers: HeaderTypes = None,
        cookies: CookieTypes = None,
        timeout: TimeoutTypes = DEFAULT_TIMEOUT_CONFIG,
        max_redirects: int = DEFAULT_MAX_REDIRECTS,
        base_url: URLTypes = None,
        trust_env: bool = True,
    ):

Would be...

DEFAULT_TIMEOUT_CONFIG = 5
DEFAULT_MAX_REDIRECTS = 20

class BaseClient:
    def __init__(
        self,
        *,
        auth: Union[Tuple[AnyStr, AnyStr], Callable[[Request], Request], Auth] = None,
        params: Union[AnyStr, Dict[AnyStr, Any], List[Tuple[AnyStr, Any]], QueryParams] = None,
        headers: Union[Dict[AnyStr, AnyStr], List[Tuple[AnyStr, AnyStr]], Headers] = None,
        cookies: Union[Dict[AnyStr, AnyStr], Cookies, CookieJar] = None,
        timeout: Union[None, float, Sequence[float], Timeout] = DEFAULT_TIMEOUT_CONFIG,
        max_redirects: int = DEFAULT_MAX_REDIRECTS,
        base_url: Union[AnyStr, URL] = None,
        trust_env: bool = True,
    ):

On one hand it's much more verbose, and we would end up needing to change annotations in multiple places when they change. On the other hand we're not having to do much tweaking now since we're fairly settled on the public API, and it at least makes the type annotations explcit up front and centre, without introducing any indirect "we're defining this this elsewhere" annotations.

Alternatively, perhaps there's something that's more of a midway point here. Eg. URLTypes seems somewhat superflous compared to using Union[AnyStr, URL] everywhere, so perhaps we could just be dropping these in case where there's not loads of excess typing?

Or else should we be making the annotation types like _TimeoutTypes, so that it's super clear they're just shorthands intended for usage in our own codebase?

(Also note - I did a bit of tweaking in the signatures for consistency & brevity. Eg. switching one of the timeout options to just Sequence[float], as a simpler and good-enough form, and using AnyStr in some places where we've currently just got str. (Eg. in the URL types. Really we ought to include the byte-wise form, since the URL is ultimately in byte-wise form.)

[StephenBrown2]

Or else should we be making the annotation types like _TimeoutTypes, so that it's super clear they're just shorthands intended for usage in our own codebase?

I think this is probably the best option. I don't mind looking up "What is a valid TimeoutType?" given the convenience of having it defined once and reused elsewhere, since it's explicit _where_ it came from in the imports at the top of the file anyways.

(Also note - I did a bit of tweaking in the signatures for consistency & brevity. Eg. switching one of the timeout options to just Sequence[float], as a simpler and good-enough form, and using AnyStr in some places where we've currently just got str. (Eg. in the URL types. Really we ought to include the byte-wise form, since the URL is ultimately in byte-wise form.)

Simplification of the types themselves though is of course definitely prudent. :+1:

[florimondmanca]

:100: agreed with @StephenBrown2 here.

[tomchristie]

Okay, so something else that I think is awkward is that the type defs are currently scattered around the codebase.

That's not entirely without reason, as the types are defined alongside the classes they include, to avoid cyclical dependencies. For example: HeaderTypes happens to be defined in models.py because it includes "Headers" as one of it's options, wheras TimeoutTypes happens to be defined in config.py, because it includes "Timeout" as one of it's options.

However it might be pretty nice if we could keep all the type definitions in a single place, eg. `typedefs.py" right? (Or at least anything that ends up in a publically exposed interface.)

I don't think I was aware of the if typing.TYPE_CHECKING: lazy imports originally, such as at https://github.com/encode/httpx/blob/b1e99a5cf9093e4c812b3895f32622f10fa8f116/httpx/utils.py#L18 - we'd likely need to use that to structure the dependant imports if we did move all the type defs into a single module.

[florimondmanca]

Sounds good too yup, similarly to Starlette’s types module which is a neat entry point for anything whose main purpose is to be used as type hints. :)

Using the TYPE_CHECKING pattern we can easily refer to dependant things as strings, so at least no technical impediment that I can think of.

[retnikt]

My issue with this strategy of making type definitions private is similar to in #855 - that you may often be writing functions which wrap HTTPX and you want the type annotations of the new function to be the same as in HTTPX, but you cannot simply declare them as httpx.whatever.SomeType because they are hidden.

This can also be problematic when there are API changes or additions where type hints change, and so you have to manually update the type hints of your wrapper functions.

[tomchristie]

I'd like us to punt this out of being considered a 1.0 blocker, since I think it's one of those kind of things that ought to have plenty of time for consideration.