Skip to content

Conversation

@clot27
Copy link
Member

@clot27 clot27 commented Jun 23, 2022

Added some missing shortcuts in Chat object

@Poolitzer
Copy link
Member

I dont like send_forward and forward_message. What about forward_from and forward_to?

And also add the missing tests please :)

@clot27
Copy link
Member Author

clot27 commented Jun 24, 2022

Mh, why the check is failing?

@Bibo-Joshi
Copy link
Member

test_official checks that ptb is up to date with the bot API, which it currently isn't (due to API 6.1). no need to worry about that :)

@clot27
Copy link
Member Author

clot27 commented Jun 24, 2022

Ready for merge? Or if anyone have noticed other missing shortcuts for Chat object, kindly tell me 👍

Comment on lines +688 to +690
assert check_shortcut_signature(Chat.forward_from, Bot.forward_message, ["chat_id"], [])
assert await check_shortcut_call(chat.forward_from, chat.get_bot(), "forward_message")
assert await check_defaults_handling(chat.forward_from, chat.get_bot())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please also add these to the new tests above

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Bibo-Joshi how did these tests not catch the return type difference (see my review), does it not check for that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

# TODO: Also check annotation of return type. Would currently be a hassle b/c typing doesn't
# resolve `ForwardRef('Type')` to `Type`. For now we rely on MyPy, which probably allows the
# shortcuts to return more specific types than the bot method, but it's only annotations after
# all

But mypy should indeed have complained 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also checked locally and mypy does indeed not pick that up … don't know why. For now I'm okay with you having catched it :D

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't know why

probably because we are ignoring return type in every bot method?

@aditya-yadav-27 reminder to implement the tests above (cause I didn't see it in the new commit)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should I add these to new set_photo, set_description, delete_photo, etc tests?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably because we are ignoring return type in every bot method?

no, I don't think that that's the reason. Bot.forward_message is annotated to return a Message - that should be all that is checked when checking what the type of await super().forward_message(…) is.

Comment on lines +688 to +690
assert check_shortcut_signature(Chat.forward_from, Bot.forward_message, ["chat_id"], [])
assert await check_shortcut_call(chat.forward_from, chat.get_bot(), "forward_message")
assert await check_defaults_handling(chat.forward_from, chat.get_bot())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Bibo-Joshi how did these tests not catch the return type difference (see my review), does it not check for that?

Returns:
:obj:`bool`: On success, :obj:`True` is returned.
.. versionadded:: 20.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please make sure that these are before the Returns block - same fore .. seealso::

@Bibo-Joshi Bibo-Joshi merged commit 7559451 into python-telegram-bot:master Jun 27, 2022
@Bibo-Joshi
Copy link
Member

Thank you for the contribution :)

@github-actions github-actions bot locked and limited conversation to collaborators Jul 4, 2022
@harshil21 harshil21 added this to the v20.0a2 milestone Jul 23, 2022
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.

4 participants