-
Notifications
You must be signed in to change notification settings - Fork 6k
adding more shortcuts #3115
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
adding more shortcuts #3115
Conversation
|
I dont like And also add the missing tests please :) |
|
Mh, why the check is failing? |
|
|
|
Ready for merge? Or if anyone have noticed other missing shortcuts for |
| 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()) |
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.
please also add these to the new tests above
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.
@Bibo-Joshi how did these tests not catch the return type difference (see my review), does it not check for that?
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.
python-telegram-bot/tests/conftest.py
Lines 502 to 505 in 76bfe8c
| # 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 🤔
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.
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
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.
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)
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.
should I add these to new set_photo, set_description, delete_photo, etc tests?
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.
yes
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.
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.
| 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()) |
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.
@Bibo-Joshi how did these tests not catch the return type difference (see my review), does it not check for that?
telegram/_chat.py
Outdated
| Returns: | ||
| :obj:`bool`: On success, :obj:`True` is returned. | ||
| .. versionadded:: 20.0 |
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.
please make sure that these are before the Returns block - same fore .. seealso::
|
Thank you for the contribution :) |
Added some missing shortcuts in
Chatobject