Repository navigation
On type annotation style. #789
Description
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.)
Activity
StephenBrown2 commentedon Jan 22, 2020
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.Simplification of the types themselves though is of course definitely prudent. 👍
florimondmanca commentedon Jan 23, 2020
💯 agreed with @StephenBrown2 here.
lovelydinosaur commentedon Jan 24, 2020
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:
HeaderTypeshappens to be defined inmodels.pybecause it includes"Headers"as one of it's options, wherasTimeoutTypeshappens to be defined inconfig.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 athttpx/httpx/utils.py
Line 18 in b1e99a5
florimondmanca commentedon Jan 24, 2020
Sounds good too yup, similarly to Starlette’s
typesmodule 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 commentedon Apr 26, 2020
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.SomeTypebecause 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.
lovelydinosaur commentedon Aug 7, 2020
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.
lovelydinosaur commentedon Apr 29, 2021
Going to close this off. Not completely against possible refactoring here, but it's not an API issue, and there's nothing pressing to be done, so...