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

Adjust read_timeout behavior for get_updates #3963

Merged
merged 15 commits into from Nov 27, 2023

Conversation

Bibo-Joshi
Copy link
Member

@Bibo-Joshi Bibo-Joshi commented Nov 4, 2023

When ready, closes #3893
Mostly as outlined in that issue. Also includes the suggestions from #3959

I decided to already change the default for read_timeout to DEFAULT_NONE, i.e. consider this a non-breaking change, because

  • from reading the documentation of ApplicationBuilder.get_updates_read_timeout, Application.run_polling, Bot.get_updates, BaseRequest.do_request, it is not directly clear which value overrides which other value, such that IMO there is no completely clear expected behavior IMO
  • in the stability policy have the clause "[…] changes of flavors of comparable behavior [are not covered]", which fits a change of the default timeout duration IMO.
  • For custom implementations of BaseRequest nothing changes

ToDo:

  • merge Improve write_timeout Handling for Media Methods #3952
  • Added .. versionadded:: NEXT.VERSION, .. versionchanged:: NEXT.VERSION or .. deprecated:: NEXT.VERSION to the docstrings for user facing changes (for methods/class descriptions, arguments and attributes)
  • Created new or adapted existing unit tests
  • Documented code changes according to the CSI standard <https://standards.mousepawmedia.com/en/stable/csi.html>__
  • Added myself alphabetically to AUTHORS.rst (optional)
  • [ ] Added new classes & modules to the docs and all suitable __all__ s
  • Checked the Stability Policy <https://docs.python-telegram-bot.org/stability_policy.html>_ in case of deprecations or changes to documented behavior

@Bibo-Joshi Bibo-Joshi requested review from Poolitzer, tsnoam and harshil21 and removed request for tsnoam November 6, 2023 19:57
@Bibo-Joshi Bibo-Joshi marked this pull request as ready for review November 8, 2023 19:51
Copy link
Member

@Poolitzer Poolitzer left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@harshil21 harshil21 left a comment

Choose a reason for hiding this comment

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

I had also gotten the same error as in #3893 while testing #3804 but I had dismissed it as an internet connection issue. Should've investigated further that time.

telegram/_bot.py Show resolved Hide resolved
telegram/request/_baserequest.py Outdated Show resolved Hide resolved
telegram/request/_httpxrequest.py Show resolved Hide resolved
telegram/ext/_application.py Outdated Show resolved Hide resolved
telegram/ext/_updater.py Outdated Show resolved Hide resolved
tests/test_bot.py Show resolved Hide resolved
tests/test_bot.py Show resolved Hide resolved
@Bibo-Joshi Bibo-Joshi added this to the 20.7 milestone Nov 23, 2023
Copy link
Member

@harshil21 harshil21 left a comment

Choose a reason for hiding this comment

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

lgtm :)

@Bibo-Joshi Bibo-Joshi merged commit da11561 into master Nov 27, 2023
23 checks passed
@Bibo-Joshi Bibo-Joshi deleted the get_updates_read_timeout branch November 27, 2023 17:24
@github-actions github-actions bot locked and limited conversation to collaborators Dec 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Exception while shutdown
3 participants