Dispatcher.set_commands #1911
Dispatcher.set_commands #1911
Conversation
|
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.
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.
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` |
| 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.
Rather just do isinstance(handler, CommandHandler). Also we should check ConversationHandlers for CommandHandlers
|
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! :) |
Proposal implementation for a feature as discussed in #1856
Tests can be simplified, once #1724 is merged.
Closes #1859