Message.reply_* to reply to same topic when in groups.
#4170
Conversation
|
Just a few thoughts if i'm allowed to: I don't see much value for having a message_thread_id param for reply_* functions. just as there isn't one for chat_id. It's a convenience method and IMO it's safe to assume that the reply should be where the original message existed. same chat, same thread/topic. |
Bibo-Joshi
left a comment
There was a problem hiding this comment.
Thank you for the PR. The code changes look good so far, looking forward to the unit tests :)
I don't see much value for having a message_thread_id param for reply_* functions. just as there isn't one for chat_id. It's a convenience method and IMO it's safe to assume that the reply should be where the original message existed. same chat, same thread/topic.
If that's not the case, user can always drop down to bot.send_*. but maybe that's too breaking of a change..soo
Apart from the "breaking change" argument, I would somewhat disagree with the statement "it's safe to assume that the reply should be where the original message existed". With the recent updates, TG has significantly broadened the term "reply" by allowing you to reply to messages in other chats and this is also supported by reply_*. Removing the ability to reply in a differen thread would be counterintuitive for me…
|
Thanks for the quick review 😄
You have a point. but for reply_* to send messages in other chats, one has to quote the message. async def start(update: Update, context: ContextTypes.DEFAULT_TYPE) -> None:
await update.message.reply_text(
"This is a reply",
do_quote=update.message.build_reply_arguments(
target_chat_id=1645307364,
),
)from within a group topic (other than general) and with |
|
Ah, interesting finding! I guess, we should use |
|
Yeah if I understand: |
There was a problem hiding this comment.
To clarify: This problem happens on your reply-same-thread branch or on master?
I don't see why it would happen on master, since so far we don't pass message_thread_id to send_message within reply_text by default.
On your branch it would make sense. In that case, I would edit the logic as indicated below - at this point it would make sense to introduce something like Message._parse_message_thread_id though :D
PS: I see no need to put such a check on values passed by the user - if they pass something that is not accepted by TG, that's their fault.
|
Hi. Just wanted to check if you need any help with the unit tests |
|
Hi, i was a bit distracted (and excited!) by the ongoing api update :). Will prioritize them. |
|
Done. Checks are failing because of the |
Bibo-Joshi
left a comment
There was a problem hiding this comment.
Awesome, thanks for the swift updates! Just a bit of nitpicking
For the tests, I can try to provide a temporary fix so that we can continue testing ...
tests/test_message.py
Outdated
| """Used in testing reply_* below. Makes sure that meassage_thread_id is parsed | ||
| correctly.""" | ||
|
|
||
| async def make_assertion(*args, **kwargs): |
There was a problem hiding this comment.
| async def make_assertion(*args, **kwargs): | |
| async def extract_message_thread_id(*args, **kwargs): |
Didn't really assert anything:)
tests/test_message.py
Outdated
| message_thread_id = await method(*args, message_thread_id=50) | ||
| assert message_thread_id == 50 | ||
|
|
||
| if bot_method_name != "send_chat_action": |
There was a problem hiding this comment.
| if bot_method_name != "send_chat_action": | |
| if bot_method_name == "send_chat_action": | |
| return |
Avoids unnecessary nesting :)
tests/test_message.py
Outdated
| target_chat_id=3, | ||
| ), | ||
| ) | ||
| assert message_thread_id == 99 |
There was a problem hiding this comment.
| target_chat_id=3, | |
| ), | |
| ) | |
| assert message_thread_id == 99 | |
| target_chat_id=message.chat_id, | |
| ), | |
| ) | |
| assert message_thread_id == message.message_thread_id |
Makes it a tad more intuitive what were testing here :)
|
apparently we don't have code coverage reports when all tests fail. I'll try and make the sticker tests xfail for now ... |
|
Thank you very much for the contribution! |
Closes #4139.
.. versionadded:: NEXT.VERSION,.. versionchanged:: NEXT.VERSIONor.. deprecated:: NEXT.VERSIONto the docstrings for user facing changes (for methods/class descriptions, arguments and attributes)Documented code changes according to theCSI standard <https://standards.mousepawmedia.com/en/stable/csi.html>__Added myself alphabetically toAUTHORS.rst(optional)Added new classes & modules to the docs and all suitable__all__sChecked theStability Policy <https://docs.python-telegram-bot.org/stability_policy.html>_ in case of deprecations or changes to documented behavior