Use timedelta to represent time periods in classes
#4750
Conversation
|
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.py tests/test_chatfullinfo.py tests/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.py tests/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 was missed (?) in #4651.
bc3cf6d to
466e0b0
Compare
|
Almost 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.
Pull Request Overview
This PR migrates numerous time-based attributes from plain integer seconds to accept and return datetime.timedelta alongside int values, enhancing API ergonomics and future-proofing duration handling.
- Introduces
TimePeriodtype alias,parse_period_arginput parsing, andget_timedelta_valueoutput normalization for all affected duration/live-period fields. - Updates docstrings with
.. versionchanged::and.. deprecated::annotations, addspropertygetters, and implementsto_dict/de_jsonto correctly serializetimedelta. - Adjusts
get_updatesin the Bot API to allowtimeoutastimedelta.
Reviewed Changes
Copilot reviewed 52 out of 52 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| telegram/_files/inputmedia.py | Added parse_period_arg and get_timedelta_value usage for duration, input media; enhanced to_dict. |
| telegram/_inline/inlinequeryresultvoice.py | Converted voice_duration to TimePeriod, added property and to_dict serialization. |
| telegram/_files/voice.py | Updated Voice.duration to accept timedelta, added de/serialization and property. |
Comments suppressed due to low confidence (3)
telegram/_files/inputmedia.py:303
- The
start_timestampfield is not passed throughparse_period_arg, so providing atimedeltawon’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
InputPaidMediaVideodefines_durationand_start_timestampbut does not overrideto_dict, so these fields won’t be serialized. Add ato_dictmethod similar to other media classes to includedurationandstart_timestampin the output.
class InputPaidMediaVideo(InputPaidMedia):
telegram/_files/inputmedia.py:34
- [nitpick] There are no tests for
InputPaidMediaVideohandling oftimedeltavs.int. Please add unit tests to verify parsing, property output, andto_dictserialization for both input types.
parse_period_arg, parse_sequence_arg
Failure at `src/telegram/_utils/datetime.py:38:0` is not related to this PR. Not sure why it's being reported just now (CI only). It's not caught locally (with fresh precommit cache).
that one's related :). I meant /stars/staramount.py:19 |
src/telegram/_telegramobject.py
Outdated
| # Converting to int here is neccassry in some cases where Bot API returns | ||
| # 'BadRquest' when expecting integers (e.g. Video.duration) | ||
| out[key] = ( | ||
| int(seconds) if (seconds := value.total_seconds()).is_integer() else seconds | ||
| ) |
There was a problem hiding this comment.
interesting catch! I'm wondering if there are cases were a float is accepted as time duration. that would be a hard-to-catch bug, if the user passes a float and the int-version is passed by TG 🤔
There was a problem hiding this comment.
Oh I meant InputMediaVideo there, will update comment.
Ah yes unfortunately :( and to make matters worse they're Business related
InputStoryContentVideo.[duration/cover_frame_timestamp]
InputProfilePhotoAnimated.main_frame_timestamp
would it suffice if we tested whether telegram returns ok if provided integers instead of floats, plus a code comment?
this also reminded me one these classes is using it's own float -> timedelta parsing logic. 👍
There was a problem hiding this comment.
ah, thanks for the info! Pointing out the currently known use cases should suffice IMO. If you want to, adding a special test case for one of them would be plenty - you could add a comment along the lines "this applies also to other classes e.g. …, but we test it only for one as this suffices to ensure that the logic works" :)
There was a problem hiding this comment.
hmm we can't really test this in the test suite, because it requires a Telegram Business account. I can prepare a minimal example to send an actual request if a team member has access to a test business account.
@Poolitzer (or others) would someone be able help verify this?
For context I want to ensure that making a request like this:
await def post_story(
self,
"business_connection_id",
content=InputStoryContentVideo(
video=video.read_bytes(),
duration=10.0, # or datetime.timedelta(seconds=10)
cover_frame_timestamp=5.0, # or datetime.timedelta(seconds=5)
is_animation=False,
),
)to telegram does not raise a BadRequest for invalid duration or cover_frame_timestamp. and a similar request for bot.set_business_account_profile_photo with photo=InputProfilePhotoAnimated.
I can run the example or provide it so that someone could :)
There was a problem hiding this comment.
mh, I don't have a business account and I fear that setting up something for them in the tests would be a hassle. Alternatively, you can mock the tg response by mocking httpx.post and returning an error if the payload contains a float
There was a problem hiding this comment.
Aha no problem!
I'm just thinking mocking the request would be of little use, because we don't know the actual Telegram behavior (throw, or not throw)
mocking httpx.post and returning an error if the payload contains a float
Also in the case of two methods above, passing a float to Telegram should be successful, it's integers that might (or might not) throw BadRequest errors.
There was a problem hiding this comment.
I do have a business account, is there an example you'd like me to run?
There was a problem hiding this comment.
Also in the case of two methods above, passing a float to Telegram should be successful, it's integers that might (or might not) throw BadRequest errors.
okay, the other way around :D
I'm just thinking mocking the request would be of little use, because we don't know the actual Telegram behavior (throw, or not throw)
That's true, it's not perfect. What we would achieve with this is that we avoid regressions in what we think is the right implementation :)
There was a problem hiding this comment.
okay, the other way around :D
haha thought i was the lost one for a sec xD
here is the test bot.
float_periods_bot.py
#!/usr/bin/env python
import asyncio
import datetime
import logging
from telegram import Update
from telegram._files._inputstorycontent import InputStoryContentVideo
from telegram._files.inputprofilephoto import InputProfilePhotoAnimated
from telegram.constants import StoryLimit
from telegram.ext import Application, CommandHandler, ContextTypes
from tests.auxil.files import data_file
logging.basicConfig(
format="%(asctime)s - %(name)s - %(levelname)s - %(message)s", level=logging.INFO
)
logging.getLogger("httpx").setLevel(logging.DEBUG)
logger = logging.getLogger(__name__)
async def post_story(update: Update, context: ContextTypes.DEFAULT_TYPE) -> None:
for duration, cover_timestamp in [
(4.0, 2.0),
(4, 2),
(datetime.timedelta(seconds=4.0), datetime.timedelta(seconds=2.0)),
]:
content = InputStoryContentVideo(
video=data_file("telegram.mp4").read_bytes(),
duration=duration,
cover_frame_timestamp=cover_timestamp,
)
logging.info("Posting story with arguments of type %s", type(duration))
logging.info("content: %s", content.to_dict())
await context.bot.post_story(
"BUSINESS_CONNECTION_ID",
content=content,
active_period=StoryLimit.ACTIVITY_SIX_HOURS,
)
await asyncio.sleep(0.5)
async def set_profile_photo(update: Update, context: ContextTypes.DEFAULT_TYPE) -> None:
"""Send a message when the command /help is issued."""
for timestamp in [
1.0,
1,
datetime.timedelta(seconds=1),
]:
photo = InputProfilePhotoAnimated(
animation=data_file("telegram2.mp4").read_bytes(),
main_frame_timestamp=1.0,
)
logging.info("Setting profile photo with arguments of type %s", type(timestamp))
logging.info("photo %s", photo.to_dict())
await context.bot.set_business_account_profile_photo(
"BUSINESS_CONNECTION_ID",
photo=photo,
is_public=False,
)
await asyncio.sleep(0.5)
def main() -> None:
"""Start the bot."""
application = Application.builder().token("TOKEN").build()
application.add_handler(CommandHandler("post_story", post_story))
application.add_handler(CommandHandler("set_profile_photo", set_profile_photo))
application.run_polling(allowed_updates=Update.ALL_TYPES)
if __name__ == "__main__":
main()thank you so, very much!
There was a problem hiding this comment.
as discussed internally here https://t.me/c/1494805131/45463, An actual request for post_story ran successfully with integers in place of floats.
tests/ext/test_updater.py
Outdated
There was a problem hiding this comment.
without these modifications the test hangs..
not sure if i missed something inside Updater.
if you notice something please note it.
src/telegram/_telegramobject.py
Outdated
| # Converting to int here is neccassry in some cases where Bot API returns | ||
| # 'BadRquest' when expecting integers (e.g. Video.duration) | ||
| out[key] = ( | ||
| int(seconds) if (seconds := value.total_seconds()).is_integer() else seconds | ||
| ) |
There was a problem hiding this comment.
ah, thanks for the info! Pointing out the currently known use cases should suffice IMO. If you want to, adding a special test case for one of them would be plenty - you could add a comment along the lines "this applies also to other classes e.g. …, but we test it only for one as this suffices to ensure that the logic works" :)
|
Thank you very much for this contribution and especially the patience in the review process! |
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.VERSIONor.. versionremoved:: NEXT.VERSIONto 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: