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

feat: add aiohttp support #16

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

aarcex3
Copy link
Contributor

@aarcex3 aarcex3 commented Sep 30, 2024

Summary

Added support for aiohttp as api client

Question

Since the aiohttp.ClientResponse has different names for common attribute (status for status_code) and also they are async attributes (e.g response.json), maybe implement a generic Response and fill it up depending on the type of client?
E.g here it will fail or give an error since aiohttp doesn't have a status_code attr, idem here for json.

Related Issue

#4

PD

I didn't implement tests since there aren't tests for the other clients

@martinn
Copy link
Owner

martinn commented Sep 30, 2024

Thank you for this! I'll have a look soon.

@martinn
Copy link
Owner

martinn commented Oct 5, 2024

Hey @aarcex3 - thank you again for this and apologies for the delayed response.

Since the aiohttp.ClientResponse has different names for common attribute (status for status_code) and also they are async attributes (e.g response.json), maybe implement a generic Response and fill it up depending on the type of client?
E.g here it will fail or give an error since aiohttp doesn't have a status_code attr, idem here for json.

That sounds like a good idea. I'm also thinking whether it would be best for the BaseHttpClient to be a protocol as well, so it can provide a bit more flexibility. What do you think?

I didn't implement tests since there aren't tests for the other clients

There are some tests for the requests client here and here (the rest all use httpx by default). At some stage I would like to re-look at tests to ideally have one core set of tests that can be easily "parametrized" to use the different http client and serializer libraries, reducing a lot of duplication. But that's a job for another day.

@aarcex3
Copy link
Contributor Author

aarcex3 commented Oct 7, 2024

That sounds like a good idea. I'm also thinking whether it would be best for the BaseHttpClient to be a protocol as well, so it can provide a bit more flexibility. What do you think?

I think we can try that

@aarcex3
Copy link
Contributor Author

aarcex3 commented Oct 7, 2024

I dunno what it should look like though.

@martinn
Copy link
Owner

martinn commented Oct 7, 2024

I dunno what it should look like though.

All good. I can have a look at that separately. It doesn't need to affect this PR.

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