Skip to content

Conversation

@seckrv
Copy link
Contributor

@seckrv seckrv commented May 5, 2018

Please refer to the commit messages for a more detailed description.

seckrv added 2 commits May 5, 2018 13:22
Dynamic handler management:
The bot API is now able to dynamically remove registered handlers
_without_ restarting. This allows e.g. the registration/deletion of new
bot features/handlers through a matrix bot command.

Additional Handler Argument:
All handlers now have an extra (3rd) argument besides (room, event).
This allows to pass information to the handler while registering. A
possible use case is a handler, which reacts in the same way on
different keywords, but needs to know about the context it was
registered in.
@shawnanastasio
Copy link
Owner

Thanks for the PR. Looks like a useful addition. My main issue is that it seems to break backwards compatibility with older bots that expect only 2 parameters in their handler functions. A way to preserve backwards compatibility without over-complicating the library would be nice. Perhaps if the kwarg arg in add_handler is not provided, then the callback function would only be called with the original two arguments. On that note, I think None is a better choice for arg's default value since it would more clearly express that the feature isn't used by default.

Also, could you explain your reasoning for adding the triggers_on methods and the get_handler method which doesn't seem to be called anywhere? The functionality for an MHandler object to communicate whether or not it is to be handled (triggered) by a given event is already exposed via the test_command method. Perhaps I'm misunderstanding their purpose, though.

Finally (nitpick), I think the example generic_callback in example_bot.py could use a slightly more descriptive name. Perhaps something along the lines of custom_greeter_callback?

Thanks,
Shawn

@MeRuslan
Copy link
Contributor

Sounds good

@seckrv
Copy link
Contributor Author

seckrv commented Jun 29, 2018

Sorry for the late reply. I was kind of busy the last weeks.
I restored backwards compatibility, making the data argument optional.

Regarding your questions:

The get_handler function enables a bot implementation to retrieve a certain
MHandler object, independent from the calling context. Imagine a command
callback (e.g. "!deactivate cheers"), which dynamically wants to deactivate the
MHandler, triggering on "cheers". In the calling context of the MHandler,
triggering on "deactivate", without get_handler the program would have no
access on the "cheers"-Handler.

The triggers_on function is needed for testing the trigger of a generic
MHandler object, as MCommandHandler implements test_command and
MRegexHandler implements test_regex. If you're iterating over an array of
MHandler objects, you can't simply call test_command on all of them, as some
may be of type MRegexHandler. Furthermore, for the invocation of
test_command/test_regex the caller needs to provide room and event, which
may not be available in every context, i want to test the trigger in. Finally,
test_command will require the cmd_char to be in the given string, wich again
may be unknown in the callers context.

Maybe my idea is now clearer. Thanks for your comments.

Cheers,
Richi

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants