Skip to content

feat: add aiohttp support#16

Draft
aarcex3 wants to merge 2 commits intomartinn:mainfrom
aarcex3:feat/aiohttp-support
Draft

feat: add aiohttp support#16
aarcex3 wants to merge 2 commits intomartinn:mainfrom
aarcex3:feat/aiohttp-support

Conversation

@aarcex3
Copy link
Copy Markdown
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
Copy Markdown
Owner

martinn commented Sep 30, 2024

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

@martinn
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

aarcex3 commented Oct 7, 2024

I dunno what it should look like though.

@martinn
Copy link
Copy Markdown
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.

@martinn
Copy link
Copy Markdown
Owner

martinn commented Oct 26, 2024

Hey @aarcex3 - let me know if there's anything I can help with on this one. Thank you!

@aarcex3
Copy link
Copy Markdown
Contributor Author

aarcex3 commented Oct 27, 2024

Hey, I thought you were gonna implement this yourself.

I can try if you want though.

@aarcex3
Copy link
Copy Markdown
Contributor Author

aarcex3 commented Oct 27, 2024

I mean this.

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.

@martinn
Copy link
Copy Markdown
Owner

martinn commented Oct 28, 2024

Hey, I thought you were gonna implement this yourself.

I can try if you want though.

If you want and can, go for it. 👍

@aarcex3
Copy link
Copy Markdown
Contributor Author

aarcex3 commented Oct 30, 2024

Hey, I wont be able to work on this.

If you want and can, go for it. 👍

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