Aiohttp request #126
Aiohttp request #126
Conversation
There was a problem hiding this comment.
Hi. Thanks for taking the time to PR for that :) I hope the authors of python-telegram-bot/python-telegram-bot#4560 appreciate it :)
- read_timeout property needs to be implemented, but this doesn't make sense since aiohttp doesn't have that timeout. Why do even have this property do we actually need it? Should I just pass the most fitting timeout like I do?
This was introduced in python-telegram-bot/python-telegram-bot#3963 and enables Bot.get_updates to use the default read timeout specified in the request object rather than hard-coding a custom default value.
- same, do_request needs all the httpx timeout params. They dont make sense at all with aiohttp, should I raise an error? I just ignore them anyway lol
IMO you should ensure that aiohttprequest.do_request(…, read_timeout=aiohttprequest.read_timeout) behaves the same as aiohttprequest.do_request(…). If aiohttp has a different set of timeout settings, then you should map our parameters to them as good as possible.
If you do not use all of them, I'd personally recommend to raise a warning (in case the passed value differs from DEFAULT_NONE) but not raise an exception.
I did not look at the tests yet
| @@ -0,0 +1,46 @@ | |||
| # AiohttpRequest instance | |||
|
|
|||
| This is an implementation of aiohttp, to be used as a replacement for httpx based on "popular" demand [#4560](python-telegram-bot/python-telegram-bot#4560). | |||
There was a problem hiding this comment.
| This is an implementation of aiohttp, to be used as a replacement for httpx based on "popular" demand [#4560](python-telegram-bot/python-telegram-bot#4560). | |
| This is an implementation of [`BaseRequest`](docs.ptb.org/…) based on [aiohttp](https://aiohttp-docs), to be used as alternative for [`HTTPXRequest`](docs.ptb.org/…) based on the request in [#4560](python-telegram-bot/python-telegram-bot#4560). |
|
|
||
|
|
||
| async def main(): | ||
| bot = telegram.Bot("TOKEN", request=AiohttpRequest()) |
There was a problem hiding this comment.
| bot = telegram.Bot("TOKEN", request=AiohttpRequest()) | |
| bot = telegram.Bot("TOKEN", request=AiohttpRequest(), get_updates_request=AiohttpRequest()) |
| from telegram._utils.logging import get_logger | ||
| from telegram._utils.types import ODVInput |
There was a problem hiding this comment.
please do not use ptb internals
| from typing import Any, Optional, Union | ||
|
|
||
| import aiohttp | ||
| import yarl |
There was a problem hiding this comment.
this is not listed in requirements.txt
| This parameter is intended for advanced users that want to fine-tune the behavior | ||
| of the underlying ``aiohttp`` clientSession. The values passed here will override | ||
| all the defaults set by ``python-telegram-bot`` and all other parameters passed to | ||
| :class:`ClientSession`. The only exception is the :paramref:`media_write_timeout` | ||
| parameter, which is not passed to the client constructor. | ||
| No runtime warnings will be issued about parameters that are overridden in this | ||
| way. |
There was a problem hiding this comment.
please update this. AiohttpRequest is no PTB class and there is also no parameter media_write_timeout here.
| # this is needed because there are errors if one uses async with or a normal def | ||
| # with ApplicationBuilder, apparently. I am confused. But it works. |
There was a problem hiding this comment.
what is "this" here? I am confused about what you're confused about.
| This makes not a lot of sense to implement since there is no actual read_timeout. | ||
| But what can I do. |
There was a problem hiding this comment.
You can write a docstring that tells the user which value you return here and why :)
| ) | ||
|
|
||
| # I dont think it makes sense to support the timeout params. | ||
| # PylintDoesntAllowMeToWriteTOLiDO if no one complains in initial PR |
| ) from err | ||
| raise TimedOut from err | ||
| except aiohttp.ClientError as err: | ||
| # HTTPError must come last as its the base httpx exception class |
There was a problem hiding this comment.
| # HTTPError must come last as its the base httpx exception class | |
| # HTTPError must come last as its the base aiohttp exception class |
?
| timeout=timeout, | ||
| data=data, | ||
| ) | ||
| except TimeoutError as err: |
There was a problem hiding this comment.
TimeoutError is not imported from anywhere
|
@Poolitzer are you still interested in working on this? |
|
yes just got busy |
This is my take on solving python-telegram-bot/python-telegram-bot#4560. I think the implementation is correct, I copied all appropriate httpx tests over.
There are open points I need feedback on:
Implementation:
Tests: