Skip to content

Conversation

@aelkheir
Copy link
Member

Closes #4085.

  • Added .. versionadded:: NEXT.VERSION, .. versionchanged:: NEXT.VERSION or .. deprecated:: NEXT.VERSION to 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 to AUTHORS.rst (optional)

  • Added new classes & modules to the docs and all suitable __all__ s

  • Checked the Stability Policy <https://docs.python-telegram-bot.org/stability_policy.html>_ in case of deprecations or changes to documented behavior

  • If relevant:

    • Added or updated documentation for the changed class(es) and/or method(s)

Copy link
Member

@Bibo-Joshi Bibo-Joshi left a 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

Comment on lines 287 to 297
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
Copy link
Member

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 🤔

Copy link
Member Author

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

or update.removed_chat_boost is not None
or update.message_reaction_count is not None
):
assert sender.id == 1
Copy link
Member

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 🤔

Copy link
Member Author

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 😅

Copy link
Member

@Bibo-Joshi Bibo-Joshi left a 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

@Bibo-Joshi Bibo-Joshi merged commit 23536ee into python-telegram-bot:master Mar 30, 2024
@Bibo-Joshi
Copy link
Member

Thank you very much for contributing this enhancement 🙂

@aelkheir aelkheir deleted the update-effective-sender branch April 1, 2024 11:28
@github-actions github-actions bot locked and limited conversation to collaborators Apr 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[FEATURE] Add Update.effective_sender

2 participants