- Notifications
You must be signed in to change notification settings - Fork5.7k
Usetimedelta
to represent time periods in classes#4750
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
aelkheir commentedApr 9, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
One thought before i proceed with the rest of the classes.
why deprecate class args, i think it would be convenient to allow passing either |
Hey, thanks for getting started so quickly:) |
Aha okay. but would it still be necessary to issue a separate deprecation warning in the init methods as was outlined in#4575 (comment) (very helpful summary btw 😄 ). one is already being issued on property access. |
I would say, so yes. Attribute access and passing the parameter are two independent operations, both of which will be affected :) |
I see! I was thinking more from the perspective that
whom are we intending to warn then anyways I did include both warnings for completeness and the cases where users are manually initializing objects |
- ChatFullInfo.slow_mode_delay.- ChatFullInfo.message_auto_delete_time.Conflicts:telegram/_chatfullinfo.pytests/test_chatfullinfo.pytests/test_official/exceptions.py
Conflicts:tests/test_chatfullinfo.py
Conflicts:tests/test_chatfullinfo.py
This includes ``duration`` param of the following:- Animation - Audio- Video - VideoNote- Voice - PaidMediaPreview- VideoChatEnded - InputMediaVideo- InputMediaAnimation - InputMediaAudio- InputPaidMediaVideo
Conflicts:tests/test_paidmedia.pytests/test_videochat.py
- InlineQueryResultGif.gif_duration- InlineQueryResultMpeg4Gif.mpeg4_duration- InlineQueryResultVideo.video_duration- InlineQueryResultAudio.audio_duration- InlineQueryResultVoice.voice_duration- InlineQueryResultLocation.live_period
- Video.start_timestamp- Poll.open_period- Location.live_period- MessageAutoDeleteTimerChanged.message_auto_delete_time- ChatInviteLink.subscription_period- InputLocationMessageContent.live_period
This also discovered `Bot.get_updates.timeout` thas wasmissed (?) in#4651.
bc3cf6d
to466e0b0
CompareAlmost ready for a review. just a few things to discuss.
Checks are really mad, will debug tomorrow :') |
Hey, thanks for the updates - looks like you've spend quite some energy in this, awesome :) I haven't managed to look at the code yet, so at least giving my 2cts on your comments
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
a simple change, but we have to change so many files and tests for this :(
PR is mostly good, I'm just double checking things-
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Pull Request Overview
This PR migrates numerous time-based attributes from plain integer seconds to accept and returndatetime.timedelta
alongsideint
values, enhancing API ergonomics and future-proofing duration handling.
- Introduces
TimePeriod
type alias,parse_period_arg
input parsing, andget_timedelta_value
output normalization for all affected duration/live-period fields. - Updates docstrings with
.. versionchanged::
and.. deprecated::
annotations, addsproperty
getters, and implementsto_dict
/de_json
to correctly serializetimedelta
. - Adjusts
get_updates
in the Bot API to allowtimeout
astimedelta
.
Reviewed Changes
Copilot reviewed 52 out of 52 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
telegram/_files/inputmedia.py | Addedparse_period_arg andget_timedelta_value usage for duration, input media; enhancedto_dict . |
telegram/_inline/inlinequeryresultvoice.py | Convertedvoice_duration toTimePeriod , added property andto_dict serialization. |
telegram/_files/voice.py | UpdatedVoice.duration to accepttimedelta , added de/serialization and property. |
Comments suppressed due to low confidence (3)
telegram/_files/inputmedia.py:303
- The
start_timestamp
field is not passed throughparse_period_arg
, so providing atimedelta
won’t populate_start_timestamp
. Replace this withself._start_timestamp = parse_period_arg(start_timestamp)
and update__slots__
accordingly.
self.start_timestamp: Optional[int] = start_timestamp
telegram/_files/inputmedia.py:212
InputPaidMediaVideo
defines_duration
and_start_timestamp
but does not overrideto_dict
, so these fields won’t be serialized. Add ato_dict
method similar to other media classes to includeduration
andstart_timestamp
in the output.
class InputPaidMediaVideo(InputPaidMedia):
telegram/_files/inputmedia.py:34
- [nitpick] There are no tests for
InputPaidMediaVideo
handling oftimedelta
vs.int
. Please add unit tests to verify parsing, property output, andto_dict
serialization for both input types.
parse_period_arg, parse_sequence_arg
@@ -110,6 +112,19 @@ def _parse_thumbnail_input(thumbnail: Optional[FileInput]) -> Optional[Union[str | |||
else thumbnail | |||
) | |||
def to_dict(self, recursive: bool = True) -> JSONDict: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
[nitpick] The conversion logic for durations to seconds is duplicated across multipleto_dict
implementations. Consider extracting this into a shared helper or mixin to DRY up the code.
Copilot uses AI. Check for mistakes.
Uh oh!
There was an error while loading.Please reload this page.
Application.run_polling, Updater.start_polling, Updater._start_polling.
Uh oh!
There was an error while loading.Please reload this page.
Second part of#4575.
Affected classes (basicaly anywhere "in seconds" appears in api docs)
Check-list for PRs
.. versionadded:: NEXT.VERSION
,.. versionchanged:: NEXT.VERSION
,.. deprecated:: NEXT.VERSION
or.. versionremoved:: NEXT.VERSION
to the docstrings for user facing changes (for methods/class descriptions, arguments and attributes)CSI standard <https://standards.mousepawmedia.com/en/stable/csi.html>
__Stability Policy <https://docs.python-telegram-bot.org/stability_policy.html>
_ in case of deprecations or changes to documented behaviorIf the PR contains API changes (otherwise, you can ignore this passage)
Checked the Bot API specific sections of the
Stability Policy <https://docs.python-telegram-bot.org/stability_policy.html>
_Created a PR to remove functionality deprecated in the previous Bot API release (
see here <https://docs.python-telegram-bot.org/en/stable/stability_policy.html#case-2>
_)If relevant: