- Notifications
You must be signed in to change notification settings - Fork5.7k
Dispatcher.set_commands#1911
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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.
LGTM
lucasmezencio commentedJul 8, 2020
Hey@Bibo-Joshi could you fix the conflicts? Let's try to push this to be merged. 🙂 |
| super(CommandHandler, self).__init__( | ||
| callback, | ||
| pass_update_queue=pass_update_queue, | ||
| pass_job_queue=pass_job_queue, | ||
| pass_user_data=pass_user_data, | ||
| pass_chat_data=pass_chat_data) | ||
| self.description = description | ||
| if self.description is not None and not 3 <= len(self.description) <= 256: | ||
| raise ValueError('Command description is not valid.') |
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 should remove this. We don't usually perform such checks as TG will just return BadRequest if invalid and more importantly, the limitations might change
| command. Defaults to :obj:`True`. | ||
| skip_empty (:obj:`bool`, optional): Whether to skip | ||
| :class:`telegram.ext.CommandHandler` s with an empty description. If :obj:`False`, | ||
| empty descriptions will be replaced by ``Command "<command>"``. |
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.
Defaults to :obj:False
| :class:`telegram.ext.CommandHandler` s with an empty description. If :obj:`False`, | ||
| empty descriptions will be replaced by ``Command "<command>"``. | ||
| alphabetical (:obj:`bool`, optional): Whether to sort commands by alphabetical order. | ||
| If :obj:`False`, commands are sorted in the same order, in with :meth:`add_handler` |
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.
with -> which
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.
Need to check what happens if a command is set twice, which can quickly be the case e.g. withCommandHandler('cancel', …) as fallback for conversations
| dp_bot_commands = [] | ||
| for group in self.handlers: | ||
| for handler in self.handlers[group]: | ||
| if hasattr(handler, 'command') and hasattr(handler, 'description'): |
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.
Rather just doisinstance(handler, CommandHandler). Also we should checkConversationHandlers forCommandHandlers
By now I'm not really confident anymore, that this is a good addition. It doesn't really save too many code lines and it probably covers only some use cases. Maybe it's better suited for an eventual contrib repo, see#1912. I'll remove from the v13 milestone for now |
tl;dr Reasoning? |
|
I only see
but I would have merged that :P |
My reasoning is basically the same as why#854 got closed: There are valid use cases for this feature, but it can only ever cover a subset of the use cases. So we can merge and find ourselfes adding more and more as users want more use cases - or we don't and save the work of maintaining a very specific feature. And at least currently I prefere the latter … Mind you, I'm still very open for#1912! :) |
Uh oh!
There was an error while loading.Please reload this page.
Proposal implementation for a feature as discussed in#1856
Tests can be simplified, once#1724 is merged.
Closes#1859