- 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-
@@ -0,0 +1,35 @@ | |||
features = "Use `timedelta` to represent time periods in classes" | |||
deprecations = """In this release, we're migrating attributes that represent durations/time periods from :ob:`int` type to Python's native :class:`datetime.timedelta`. |
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.
deprecations ="""In this release, we're migrating attributes that represent durations/time periods from :ob:`int` type to Python's native :class:`datetime.timedelta`. | |
deprecations ="""In this release, we're migrating attributes that represent durations/time periods from :obj:`int` type to Python's native :class:`datetime.timedelta`. |
features = "Use `timedelta` to represent time periods in classes" | ||
deprecations = """In this release, we're migrating attributes that represent durations/time periods from :ob:`int` type to Python's native :class:`datetime.timedelta`. | ||
Enable the ``PTB_TIMEDELTA`` environment variable to adopt :obj:`timedelta` now. Support for :obj:`int` values is deprecated and will be removed in a future major version. |
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.
Enable the ``PTB_TIMEDELTA`` environment variable to adopt :obj:`timedelta` now. Support for :obj:`int` values is deprecated and will be removed in a future major version. | |
Set the ``PTB_TIMEDELTA=1`` environment variable to adopt :obj:`timedelta` now. Support for :obj:`int` values is deprecated and will be removed in a future major version. |
it's not clear to me what happens when you set the env variable, are timedelta objects returned? If that's the case, the release note should mention that.
@@ -101,3 +101,5 @@ | |||
.. |org-verify| replace:: `on behalf of the organization <https://telegram.org/verify#third-party-verification>`__ | |||
.. |time-period-input| replace:: :class:`datetime.timedelta` objects are accepted in addition to plain :obj:`int` values. | |||
.. |time-period-int-deprecated| replace:: In a future major version this will be of type :obj:`datetime.timedelta`. You can opt-in early by setting the `PTB_TIMEDELTA` environment variable. |
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.
.. |time-period-int-deprecated|replace:: In a future major version this will be of type:obj:`datetime.timedelta`. You can opt-in early by settingthe`PTB_TIMEDELTA` environment variable. | |
.. |time-period-int-deprecated|replace:: In a future major version this will be of type:obj:`datetime.timedelta`. You can opt-in early by setting `PTB_TIMEDELTA=1` as an environment variable. |
read_timeout = ( | ||
(arg_read_timeout + timeout.total_seconds()) | ||
if isinstance(timeout, dtm.timedelta) | ||
else (arg_read_timeout + timeout if isinstance(timeout, int) else arg_read_timeout) | ||
) |
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.
read_timeout= ( | |
(arg_read_timeout+timeout.total_seconds()) | |
ifisinstance(timeout,dtm.timedelta) | |
else (arg_read_timeout+timeoutifisinstance(timeout,int)elsearg_read_timeout) | |
) | |
read_timeout= ( | |
(arg_read_timeout+timeout.total_seconds()) | |
ifisinstance(timeout,dtm.timedelta) | |
else (arg_read_timeout+timeoutiftimeoutelsearg_read_timeout) | |
) |
ah I don't think I've ever seen nested ternary if statements in python haha. Regarding the change, we should check for truthiness oftimeout
like before, because your case didn't cover if timeout was a float.
def get_timedelta_value(value: Optional[dtm.timedelta]) -> Optional[Union[float, dtm.timedelta]]: | ||
""" | ||
Convert a `datetime.timedelta` to seconds or return it as-is, based on environment config. |
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.
should mention here that it returns a timedelta if the env var is set, otherwise a float.
@@ -80,18 +93,21 @@ class Video(_BaseThumbedMedium): | |||
the video in the message. | |||
.. versionadded:: 21.11 | |||
start_timestamp (:obj:`int`): Optional, Timestamp in seconds from which the video | |||
will play in the message | |||
start_timestamp (:obj:`int` | :class:`datetime.timedelta`): Optional, Timestamp in seconds |
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.
start_timestamp (:obj:`int`| :class:`datetime.timedelta`):Optional,Timestampinseconds | |
start_timestamp (:obj:`int`| :class:`datetime.timedelta`):Optional.Timestampinseconds |
@property | ||
def duration(self) -> Union[int, dtm.timedelta]: |
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.
I might be wrong, but I think we have to add a docstring for the properties here and remove it from the Attributes section up top. If you can build the docs and check how they look and confirm if they're good that would be nice.
assert animation_dict["thumbnail"] == animation.thumbnail.to_dict() | ||
assert animation_dict["file_name"] == animation.file_name | ||
assert animation_dict["mime_type"] == animation.mime_type | ||
assert animation_dict["file_size"] == animation.file_size | ||
def test_time_period_properties(self, PTB_TIMEDELTA, animation): |
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.
I don't think it's idiomatic to have an all caps fixture name in the argument. Maybe rename it?
@pytest.fixture(scope="module", params=["true", "false", None]) | ||
def PTB_TIMEDELTA(request): | ||
# Here we manually use monkeypatch to give this fixture module scope | ||
monkeypatch = pytest.MonkeyPatch() |
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.
why doesn't specifying monkeypatch as a parameter work?
"TransactionPartnerUser": { | ||
"subscription_period": int, # actual: Union[int, dtm.timedelta] |
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.
do we still need this?
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.
def to_dict(self, recursive: bool = True) -> JSONDict: | ||
"""See :meth:`telegram.TelegramObject.to_dict`.""" | ||
out = super().to_dict(recursive) | ||
if isinstance(self, (InputMediaAnimation, InputMediaVideo, InputMediaAudio)): |
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.
The genericto_dict
only handlesInputMediaAnimation
,InputMediaVideo
, andInputMediaAudio
. It should also includeInputPaidMediaVideo
so its_duration
and_start_timestamp
fields are properly emitted.
ifisinstance(self, (InputMediaAnimation,InputMediaVideo,InputMediaAudio)): | |
ifisinstance(self, (InputMediaAnimation,InputMediaVideo,InputMediaAudio,InputPaidMediaVideo)): |
Copilot uses AI. Check for mistakes.
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: