-
Notifications
You must be signed in to change notification settings - Fork 6k
Add property Update.effective_sender
#4168
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
Add property Update.effective_sender
#4168
Conversation
Bibo-Joshi
left a comment
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.
Hi, thanks for the PR! Overall change quality looks good, just left a few comments
| def test_effective_sender_anonymous(self, update): | ||
| # remove none anonymous user | ||
| if message := (update.message or update.edited_message): | ||
| message._unfreeze() | ||
| message.from_user = None | ||
| elif reaction := (update.message_reaction): | ||
| reaction._unfreeze() | ||
| reaction.user = None | ||
| elif answer := (update.poll_answer): | ||
| answer._unfreeze() | ||
| answer.user = None |
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.
TBH I don't quite get what this test is covering that the above tests does not cover 🤔
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.
ahhh, here i was trying to mimic updates from anonymous group members. for example a reaction coming from anonymous user would have actor_chat and not user. but since they're both set, i remove user.
this may allow us to do assert isinstance(sender, Chat) you mentioned
tests/test_update.py
Outdated
| or update.removed_chat_boost is not None | ||
| or update.message_reaction_count is not None | ||
| ): | ||
| assert sender.id == 1 |
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 would have expected something like assert isinstance(sender, Chat/User) here making sure that we're getting the right attribute for the right use cases 🤔
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.
Right. this is why i intended to have two tests, although currently not working as you suggest..
test_effective_sender_non_anonymous: makes sure sender is either User or None
test_effective_sender_anonymous: makes sure sender is either Chat, User or None
i'll make them them more clear 😅
Bibo-Joshi
left a comment
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.
Thanks for the updates! One more comment
|
Thank you very much for contributing this enhancement 🙂 |
Closes #4085.
Added
.. versionadded:: NEXT.VERSION,.. versionchanged:: NEXT.VERSIONor.. deprecated:: NEXT.VERSIONto the docstrings for user facing changes (for methods/class descriptions, arguments and attributes)Created new or adapted existing unit tests
Documented code changes according to the
CSI 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 behaviorIf relevant: