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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Message.reply_*
to reply to same topic when in groups.
#4170
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. |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.VERSION
or.. deprecated:: NEXT.VERSION
to 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