Skip to content

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 added bug 🐛 📋 do-not-merge-yet work status: do-not-merge-yet 🛠 refactor change type: refactor labels Nov 4, 2023
@Bibo-Joshi Bibo-Joshi requested review from Poolitzer, harshil21 and tsnoam 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.

@Bibo-Joshi Bibo-Joshi added this to the 20.7 milestone Nov 23, 2023
@Bibo-Joshi Bibo-Joshi requested a review from harshil21 November 23, 2023 18:59
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
@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
@Bibo-Joshi Bibo-Joshi added 🔌 bug pr description: bug and removed bug 🐛 labels Nov 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

🔌 bug pr description: bug 📋 do-not-merge-yet work status: do-not-merge-yet 🛠 refactor change type: refactor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Exception while shutdown

4 participants