Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

narrow client request methods down #303

Closed
wants to merge 1 commit into from

Conversation

cansarigol
Copy link
Contributor

#174
Hi, I tried to narrow Client methods down with TypeDict. However, does not seem possible until the issue fixed. This code may be useful.

@cansarigol cansarigol force-pushed the narrow-client-methods branch 3 times, most recently from d3a0cce to 41b7392 Compare September 1, 2019 22:00
@florimondmanca
Copy link
Member

Interesting take, thanks! It’s great to see a possible solution to better typing support of **kwargs.

How well does this work with autocompletion, though? I suppose this was the main motivation for repeating the keyword arguments in the client methods.

I think there’s a big blocker, though — TypeDict is a new, 3.8-only feature of the typing module, but we support 3.6 and up. So I don’t think this will make it but I really appreciate you giving this a shot.

(Though, I’m not sure the approach demonstrated here was the idea behind #174. We definitely need to discuss the proposed scope and API of build_request() there.)

@cansarigol
Copy link
Contributor Author

cansarigol commented Sep 2, 2019

Thanks for review.

How well does this work with autocompletion, though? I suppose this was the main motivation for repeating the keyword arguments in the client methods.

I've been trying to fix two things. First, use one line client functions and decorate them with a generic function to handle their signature. Second, when mypy runs, it introspects generic arguments from kwargs.

I don't know how IDEs handle moving autocomplete arguments. added a decorator to override client function's signature. With bind or bind_partial functions, I can check their arguments in runtime but in design time it doesn't work. I use vscode but I'm not sure about Pycharm.

I think there’s a big blocker, though — TypeDict is a new, 3.8-only feature of the typing module, but we support 3.6 and up. So I don’t think this will make it but I really appreciate you giving this a shot.

Are you sure about this? because it comes from mypy_extensions module and I didn't get any error.

@florimondmanca
Copy link
Member

Are you sure about this? because it comes from mypy_extensions module and I didn't get any error.

Oh, indeed. Cool!

First, use one line client functions and decorate them with a generic function to handle their signature.

From experience, signature-altering decorators don't play that well with IDEs (because those generally rely on the initial signature of the function). I'll give it a go in VSCode when I have some time, and we'll see. :)

@cansarigol
Copy link
Contributor Author

by the way, I came across this issue I'm watching it.

@florimondmanca
Copy link
Member

VSCode isn't giving me any autocompletions, and IPython can't see the signature either:

Screenshot 2019-09-02 at 09 21 44

Compare with httpx.get:

Screenshot 2019-09-02 at 09 22 16

Copy link
Member

@florimondmanca florimondmanca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, I'm not sure this is something we even want to have. Sure, repeating the arguments looks cumbersome but it's not that much of a maintenance burden, it makes very explicit what these methods accept, and it gives a nice editor experience.

(Edit: there's also the question of what we would do with the AsyncClient's methods? Really, I think we're fine with things are they are today.)

Would love to have @sethmlarson's point of view on this. :-)

httpx/decorators.py Outdated Show resolved Hide resolved
@cansarigol
Copy link
Contributor Author

at least I added share_signature 😄

>>> import httpx
>>> httpx.Client.get.__signature__
<Signature (self, method: str, url: Union[ForwardRef('URL'), str], *, data: Union[dict, str, bytes, Iterator[bytes]] = None, files: Dict[str, Union[IO[
~AnyStr], Tuple[str, IO[~AnyStr]], Tuple[str, IO[~AnyStr], str]]] = None, json: Any = None, params: Union[ForwardRef('QueryParams'), Mapping[str, Union
[str, int, float, bool, NoneType]], List[Tuple[str, Union[str, int, float, bool, NoneType]]], str] = None, headers: Union[ForwardRef('Headers'), Dict[~
AnyStr, ~AnyStr], List[Tuple[~AnyStr, ~AnyStr]]] = None, cookies: Union[ForwardRef('Cookies'), http.cookiejar.CookieJar, Dict[str, str]] = None, stream
: bool = False, auth: Union[Tuple[Union[str, bytes], Union[str, bytes]], Callable[[ForwardRef('AsyncRequest')], ForwardRef('AsyncRequest')]] = None, al
low_redirects: bool = True, cert: Union[str, Tuple[str, str], Tuple[str, str, str]] = None, verify: Union[str, bool, ssl.SSLContext] = None, timeout: U
nion[float, Tuple[float, float, float], ForwardRef('TimeoutConfig')] = None, trust_env: bool = None) -> httpx.models.Response>
>>> 

@florimondmanca
Copy link
Member

So, I’m going to close this because I think it’s not something we need, esp with respect to maintaining the readability, ease of maintenance and editor support of the client. But thanks again for trying it out! (In the future, feel free to open issues to discuss this kind of idea, if you’d like a quick feedback on whether it’s something worth working on.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants