locustio
Httpsession requests typing
The aim of this PR is to provide better typing annotation support for class HttpSession.
This should also fix #2563.
This PR is still not finished (in draft). Requires adding similar annotations to FastHttpSession and more testing. It would be nice and beneficial if you @cyberw could express if you are happy with this direction of changes.
Is it possible to put all the override definitions in a if typing_checking-block as well? I'd like to avoid the extra level of indirection at run time.
I'm not 100% sure, but according to my current understanding of Python typing ecosystem — no. I'll do more research about it.
I'd like to avoid the extra level of indirection at run time.
What is the main reason behind it? Easier debugging / performance — necessity to create extra stack frame for additional function call / something else?
Source code of methods in the base class is very short:
def get(self, url, **kwargs):
kwargs.setdefault("allow_redirects", True)
return self.request("GET", url, **kwargs)
def options(self, url, **kwargs):
kwargs.setdefault("allow_redirects", True)
return self.request("OPTIONS", url, **kwargs)
def head(self, url, **kwargs):
kwargs.setdefault("allow_redirects", False)
return self.request("HEAD", url, **kwargs)
def post(self, url, data=None, json=None, **kwargs):
return self.request("POST", url, data=data, json=json, **kwargs)
def put(self, url, data=None, **kwargs):
return self.request("PUT", url, data=data, **kwargs)
def patch(self, url, data=None, **kwargs):
return self.request("PATCH", url, data=data, **kwargs)
def delete(self, url, **kwargs):
return self.request("DELETE", url, **kwargs)
What do you think about following possible implementation in Locust:
def get(self, url: str | bytes, **kwargs: Unpack[RESTKwargs]): # type: ignore[override]
kwargs.setdefault("allow_redirects", True)
return self.request("GET", url, **kwargs) # type: ignore[misc]
The number of function call would remain the same.
Yes, I’m mostly thinking about runtime performance. Why doesnt it work to define the wrappers in an if-block?
I think about it as defining types of method that de facto doesn't exist (in contrast to a function with the same name in a base class). I doubt if type checkers were designed to handle such cases. But I'm just guessing. I don't know. I'm going to check it.
I will do it this weekend.
On Tue, 4 Jun 2024, 20:53 Lars Holmberg, @.***> wrote:
hi @tdadela https://github.com/tdadela did you have a chance to try it out? Or should I try it?
— Reply to this email directly, view it on GitHub https://github.com/locustio/locust/pull/2699#issuecomment-2148195004, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJM4KUOWQAYPRYWYS4S7BMLZFYEJ3AVCNFSM6AAAAABHHFTPXCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNBYGE4TKMBQGQ . You are receiving this because you were mentioned.Message ID: @.***>
Sorry for the delay. I tried defining HttpSession with annotations, inside if TYPE_CHECKING. It was ignored by type checkers.
I see 3 options:
- Keeping HttpSession as is. (no changes, closing PR)
- Trying to define HttpSession with annotations in a .pyc file.
- Calling self.requests directly inside HttpSession REST methods (proposition implemented in the last commit). This doesn't increase the number of function calls.
I like the version in the PR now. Have you tried it with vscode/pyright?
Maybe add a constraint to the typing_extensions dependency to only download it on Python versions <3.11?
Hi, I'm using pyright 1.1.338 in NeoVim. (With Python version 3.12.3). tdadela's version seems to work great. The catch_response and name kwargs weren't type hinted correctly before, now they are. Some screenshots:
The only downside is that the method signature becomes less informative in my editor.
Before:
After:
That's bad default behaviour on the part of the LSP, which should unwrap
RESTKwargs IMO. Probably the only way to fix that would be to explicitly type the kwargs for each HTTP method.
Hope this helps. Thanks for the awesome project and great work tdadela!
@cyberw Do you prefer the last or the last but one version? (I'll fix the stupid mistake in a minute)
Also, I've realized that # type: ignore[misc] is no longer necessary, probably since calling self.request directly. I'll remove it later. (To not make the impression it's connected with the last commit).
Have you tried it with vscode/pyright?
Yes. It works well. (I hope I checked everything correctly.)
@0scarB Thank you very much for your support! It's especially beneficial since I don't use some tools, e.g., pyright on daily bases (I'm not sure if I test everything correctly).
@0scarB Thank you very much for your support! It's especially beneficial since I don't use some tools, e.g., pyright on daily bases (I'm not sure if I test everything correctly).
Tested the latest commit in my project. LGTM! The return types are now correctly shown by pyright. Happy to help
Looks good to me too. @tdadela If you feel it is ready to go, move it out of draft and I'll merge it. Ideally I'd like to use the same typing for FastHttpSession but we can do that later on...
Today I tried to make return type argument dependent (catch_response) using @overload. Unfortunately, this makes the file super clutter.
Ideally I'd like to use the same typing for FastHttpSession but we can do that later on...
Yup, soon in another PR.