Implement react for TerminalBotHandler #688
Conversation
zulip_bots/zulip_bots/simple_lib.py
Outdated
| @@ -39,15 +39,22 @@ def update(self, message): | |||
| def upload_file(self, file): | |||
| return dict(result='success', msg='', uri='https://server/user_uploads/{}'.format(uuid4())) | |||
|
|
|||
| class TerminalBotHandler: | |||
| class TerminalBotHandler(BotHandler): | |||
| def __init__(self, bot_config_file): | |||
There was a problem hiding this comment.
I don't think TerminalBotHandler needs to inherit BotHandler, by default all BotHandlers follow the protocol.
I think the subsequent changes made in this commit are enough for that.
There was a problem hiding this comment.
Hmm, I kinda feel like this should inherit from AbstractBotHandler once we do that rename.
There was a problem hiding this comment.
If we inherit a protocol, all properties of the protocol now come into the class even if the class does not implement it. Since the implementation of abstract functions is not a mandate. I would rather recommend creating functions and not inheriting them so that we can come back to them later. Inheriting a Protocol also skips a lot of mypy issues like the ones we faced here which gets caught, if we keep them separate.
There was a problem hiding this comment.
I've not looked deeply at these recent changes but if this is intended to be a Protocol (~ static duck typing), then I agree with @LoopThrough-i-j re not inheriting. It may make it clearer to add a Protocol suffix to BotHandler to indicate that.
This makes the user and the bot to share the message server when sending messages. As a result, the message id can be shared. And history messages sent by the user will be stored as well.
|
I had a look and things look good. @neiljp would you mind reviewing? |
|
This lgtm, merged, thanks @PIG208 and @LoopThrough-i-j! I think we should do a round of adding docstring/comment documentation to TerminalBotHandler explaining what's going on -- e.g. the (We might also want to rename |
Fixes #686.