[REFACTOR] Shortcut bots handling#2712
Conversation
Bibo-Joshi
left a comment
There was a problem hiding this comment.
Thanks for the PR! I've added a few comments below :)
Bibo-Joshi
left a comment
There was a problem hiding this comment.
The rest of the updates look good :) One thing that I forgot about so far: We should add new unit tests. please add a test to test_telegramobject.py that make sure that:
- calling
get_bot()without having set a bot raisesRuntimeError - calling
get_bot()after having set a bot instance withset_botreturns the same object - calling
set_bot(None)after that leads toget_bot()raisingRuntimeErroragain
There was a problem hiding this comment.
Thanks for the updates! I left just some minor comments :) Besides that, can you merge v14 into your branch to resolve the merge conflicts? #2687 just got merged, which renamed a lot of file (but changed almost nothing in the files). If you're e.g. using pycharm, merging shouldn't bee too hard. If you're having troubles with this, please tell us :)
telegram/_passport/data.py
Outdated
| self.expiry_date = expiry_date | ||
|
|
||
| self.bot = bot | ||
| self._bot = bot |
There was a problem hiding this comment.
Maybe Im stupid, why is this different from the other classes?
There was a problem hiding this comment.
It shouldn't have been.
Fixed.
telegram/_telegramobject.py
Outdated
| if self._bot is None: | ||
| raise RuntimeError( | ||
| 'This object has no bot associated with it. \ | ||
| Shortcuts cannot be used' |
There was a problem hiding this comment.
| Shortcuts cannot be used' | |
| Shortcuts cannot be used.' |
tests/test_audio.py
Outdated
| assert check_shortcut_signature(Audio.get_file, Bot.get_file, ['file_id'], []) | ||
| assert check_shortcut_call(audio.get_file, audio.bot, 'get_file') | ||
| assert check_defaults_handling(audio.get_file, audio.bot) | ||
| assert check_shortcut_call(audio.get_file, audio._bot, 'get_file') |
There was a problem hiding this comment.
shouldnt we use get_bot here as well?
tests/test_shippingquery.py
Outdated
| assert shipping_query.from_user == self.from_user | ||
| assert shipping_query.shipping_address == self.shipping_address | ||
| assert shipping_query.bot is bot | ||
| assert shipping_query._bot is bot |
There was a problem hiding this comment.
also, shouldn't this be get_bot?
|
Thanks for the contribution. |
Addresses #2505.
Checklist for PRs
.. versionadded:: version,.. versionchanged:: versionor.. deprecated:: versionto the docstrings for user facing changes (for methods/class descriptions, arguments and attributes)AUTHORS.rst(optional)Chat&Userfor all methods that acceptchat/user_idMessagefor all methods that acceptchat_idandmessage_idMessageshortcuts: Addedquoteargument if methods acceptsreply_to_message_idCallbackQueryfor all methods that accept eitherchat_idandmessage_idorinline_message_idI'd like to draw your attention to this line as
test_slot_behaviourfails with "AssertionError: got extra slot '_bot'without it